-
Notifications
You must be signed in to change notification settings - Fork 33
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
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
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. |
|
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. |
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? |
I wonder why this works though? https://github.com/Ferrite-FEM/Ferrite.jl/blob/b56e2c9f6dce430a4fcbffee2246449731df2f93/src/Export/VTK.jl#L118-L122 Is this another code path? |
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. |
I hadn't realized that MeshCell isn't concrete anymore. I think the code in Ferrite was written back when it was.
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. |
Fixes #155