Skip to content

Feature: [vtk][reader] Support polylines#307

Merged
dglaeser merged 1 commit into
mainfrom
feature/polylines
Mar 23, 2025
Merged

Feature: [vtk][reader] Support polylines#307
dglaeser merged 1 commit into
mainfrom
feature/polylines

Conversation

@timokoch
Copy link
Copy Markdown
Collaborator

@timokoch timokoch commented Mar 21, 2025

Add basic support for VTK polylines in the reader. Polylines with two corners are converted to a segment, so there should be no change in behavior. This does not propose yet to actually handle polylines in any of the implemented adapters. The change in the Dune traits deals with an error from the CI test suite because not all enum states are handled in a switch statement (I guess that's because of the pedantic settings in the CI?).

Based on this PR I was able to write a custom reader that handles polylines and data by splitting polylines into segments.

@github-actions
Copy link
Copy Markdown

The following performance differences (this branch vs target branch) have been measured:
Relative deviation of average for 'app_raw': 0.90%
Relative deviation of average for 'inline_b64': -1.83%
Relative deviation of average for 'app_b64': -0.41%
Relative deviation of average for 'ascii': -0.45%

@timokoch timokoch changed the title Draft: [vtk][reader] Support polylines Feature: [vtk][reader] Support polylines Mar 21, 2025
@timokoch timokoch force-pushed the feature/polylines branch from 023e3d4 to f967893 Compare March 21, 2025 11:06
@github-actions
Copy link
Copy Markdown

The following performance differences (this branch vs target branch) have been measured:
Relative deviation of average for 'inline_b64': -0.38%
Relative deviation of average for 'app_raw': -0.70%
Relative deviation of average for 'ascii': 0.48%
Relative deviation of average for 'app_b64': 0.04%

@timokoch timokoch requested a review from dglaeser March 21, 2025 11:31
@timokoch timokoch added the enhancement New feature or request label Mar 21, 2025
@dglaeser
Copy link
Copy Markdown
Owner

dglaeser commented Mar 22, 2025

Thanks a lot for this contribution @timokoch! I am wondering though, after introducing a new cell type, I should probably also adapt the VTPWriter to support polylines? Could also open an issue and do that in a follow-up PR though.

EDIT: yes I just checked the writer, it should throw an exception when polylines exist in the grid.

@dglaeser
Copy link
Copy Markdown
Owner

dglaeser commented Mar 22, 2025

This does not propose yet to actually handle polylines in any of the implemented adapters

So I think the dune traits are currently the only ones that provide something for reading in grids, the other traits only provide adapters for writing, and no writers support polylines so far (see #309). So this should be fine.

The change in the Dune traits deals with an error from the CI test suite because not all enum states are handled in a switch statement (I guess that's because of the pedantic settings in the CI?).

Interesting, I would have thought the default case would be enough. But it's good to explicitly handle the case.

Based on this PR I was able to write a custom reader that handles polylines and data by splitting polylines into segments.

Do you think it makes sense to put this change into the dune grid factory provided with the gridformat traits? There is no Dune::GeometryType::Polyline right? So it could make sense to have this as a default behaviour for the dune traits provided here? Then you also would not need your custom VTPReader in https://git.iws.uni-stuttgart.de/dumux-repositories/dumux/-/merge_requests/3934 right?

@timokoch
Copy link
Copy Markdown
Collaborator Author

Do you think it makes sense to put this change into the dune grid factory provided with the gridformat traits? There is no Dune::GeometryType::Polyline right? So it could make sense to have this as a default behaviour for the dune traits provided here? Then you also would not need your custom VTPReader in https://git.iws.uni-stuttgart.de/dumux-repositories/dumux/-/merge_requests/3934 right?

I have a custom Factory adapter. That one is very slim. But the factory does not affect the way data fields are read. So I got a segfault because sizes don’t match up. For the fields I implemented the custom reader (essentially modifies ‘_num_cells()’ and ‘_get_cell_field(…)’). Is there a better way?

@dglaeser
Copy link
Copy Markdown
Owner

Do you think it makes sense to put this change into the dune grid factory provided with the gridformat traits? There is no Dune::GeometryType::Polyline right? So it could make sense to have this as a default behaviour for the dune traits provided here? Then you also would not need your custom VTPReader in https://git.iws.uni-stuttgart.de/dumux-repositories/dumux/-/merge_requests/3934 right?

I have a custom Factory adapter. That one is very slim. But the factory does not affect the way data fields are read. So I got a segfault because sizes don’t match up. For the fields I implemented the custom reader (essentially modifies ‘_num_cells()’ and ‘_get_cell_field(…)’). Is there a better way?

I think on the reader side, it is fine to leave it as is, because the cells are polylines, so according to the grid file and the specified cell type, that is the number of cells, right? For instance, for cell fields, VTK would render all subsegments in the polyline with the same color right (i.e. the cell values are the same for each subsegment).

But we could add an option to vtp, e.g. subdivide_polylines to make that behaviour explicit.

@dglaeser dglaeser force-pushed the feature/polylines branch from f6d4132 to f967893 Compare March 22, 2025 22:33
@github-actions
Copy link
Copy Markdown

The following performance differences (this branch vs target branch) have been measured:
Relative deviation of average for 'app_raw': -1.36%
Relative deviation of average for 'app_b64': -0.66%
Relative deviation of average for 'ascii': 2.17%
Relative deviation of average for 'inline_b64': -1.04%

@github-actions
Copy link
Copy Markdown

The following performance differences (this branch vs target branch) have been measured:
Relative deviation of average for 'inline_b64': -0.52%
Relative deviation of average for 'app_b64': -0.78%
Relative deviation of average for 'ascii': -0.32%
Relative deviation of average for 'app_raw': -0.21%

@dglaeser dglaeser merged commit 49b075f into main Mar 23, 2025
@dglaeser dglaeser deleted the feature/polylines branch March 23, 2025 00:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants