Skip to content

Conversation

@jipolanco
Copy link
Member

Fixes #155

@codecov
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

3 participants