Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Throw informative error if cell vector is type unstable #156

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jipolanco
Copy link
Member

Fixes #155

Copy link

codecov bot commented Nov 5, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 96.29%. Comparing base (9b5c7ed) to head (bd8b593).

Files with missing lines Patch % Lines
src/gridtypes/unstructured/unstructured.jl 50.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #156      +/-   ##
==========================================
- Coverage   96.92%   96.29%   -0.64%     
==========================================
  Files          15       15              
  Lines         879      891      +12     
==========================================
+ Hits          852      858       +6     
- Misses         27       33       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fredrikekre
Copy link
Contributor

identity.(vec) can be used to "tighten" the type

@fredrikekre
Copy link
Contributor

On the other hand, it might be better to show a good error message since this could be a performance trap? E.g. the code in here would essentially copy the array, and the user code is also probably slower than it could be.

@jipolanco
Copy link
Member Author

Yes, my point of view was that, when user code is already type unstable, then we don't need to care too much about performance and thus making a copy is OK.

But it's true that in many cases the user doesn't know their code is type unstable, so showing an error or a warning can make sense. Perhaps we can show an error describing the "correct" way of initialising a vector of cells?

@jipolanco jipolanco changed the title Allow passing cells in non-concrete container type Throw informative error if cell vector is type unstable Nov 6, 2024
@fredrikekre
Copy link
Contributor

@jipolanco
Copy link
Member Author

Because of this:

# By default, cells are attached to unstructured grids.
grid_type(::Type{<:AbstractMeshCell}) = VTKUnstructuredGrid()

This means that, by default, unstructured files (.vtu) are created when the cell type is not fully inferred. This works most of the time, except when a polydata file (.vtp) should be created instead (as in #155).

So this PR may actually break a lot of code which currently works including Ferrite.

@fredrikekre
Copy link
Contributor

I hadn't realized that MeshCell isn't concrete anymore. I think the code in Ferrite was written back when it was.

So this PR may actually break a lot of code which currently works including Ferrite.

Perhaps we can add some basic downstream CI here similar to e.g. https://github.com/Ferrite-FEM/Ferrite.jl/blob/master/.github/workflows/Downstream.yml . Running the full testsuite of Ferrite is definitely overkill, but we can at least run some basic export stuff perhaps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error: type Polys has no field vtk_id
2 participants