WriteVTK.jl icon indicating copy to clipboard operation
WriteVTK.jl copied to clipboard

Throw informative error if cell vector is type unstable

Open jipolanco opened this issue 1 year ago • 7 comments

Fixes #155

jipolanco avatar Nov 05 '24 16:11 jipolanco

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 :warning:
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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Nov 05 '24 16:11 codecov[bot]

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

fredrikekre avatar Nov 05 '24 17:11 fredrikekre

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.

fredrikekre avatar Nov 06 '24 10:11 fredrikekre

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 avatar Nov 06 '24 10:11 jipolanco

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?

fredrikekre avatar Nov 06 '24 12:11 fredrikekre

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.

jipolanco avatar Nov 06 '24 12:11 jipolanco

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.

fredrikekre avatar Nov 07 '24 08:11 fredrikekre