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

Fix 2D position computation #1047

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from

Conversation

eve-le-guillou
Copy link
Contributor

Hi all,

This PR fixes an incoherence in triangleToPosition2D.
Originally, it would output the position in terms of triangles (and not VTK squares).
The problem is that its 3D counterpart tetrahedronToPosition, outputs the number of VTK cubes. This creates an incoherence when both methods are used within the same code.

Dividing p[0] in triangleToPosition2D makes the method output squares. However, this means changing as well getCellVertex, so that it does not divide by two for the 2D case.

This was detected due to the use of both functions in MPI features.

To test that these changes don't impact more of the code, I computed the DiscreteGradient on the cells.vti dataset. No differences have been detected when comparing the results with and without the changes.

Thanks for any feedback,

@julien-tierny
Copy link
Collaborator

thanks a lot @eve-le-guillou !
it looks all good to me.
@pierre-guillou are you OK with this?

@pierre-guillou
Copy link
Contributor

You might also need to modify the ImplicitTriangulationCRTP::getTriangleEdgeInternal method, playing with the TriangulationRequest filter with Simplex "Triangle" and Request Type "Facet" outputs wrong edges.

The ImplicitTriangulationCRTP::getTriangleNeighborNumber and ImplicitTriangulationCRTP::getTriangleNeighbor methods have similar p[0] / 2 expressions, they might need additional correction too.

@eve-le-guillou
Copy link
Contributor Author

Hi,

I added the recommended changes and also changed the corresponding methods in the periodic triangulation (that also showed some /2 on the 0 coordinate in 2D).

It should be good now.

@julien-tierny
Copy link
Collaborator

hi @eve-le-guillou
I was double-checking this PR this morning and I believe that there are still some issues with periodic implicit 2D grids.
I have the impression that non-periodic implicit grids are fine (the states timeTracking and uncertainStartingVortex compute fine).
the state clusteringKelvinHelmholtzInstabilities.pvsm (https://topology-tool-kit.github.io/examples/clusteringKelvinHelmholtzInstabilities/) is the only one that uses periodic implicit 2D grids.

when I run that state with TTK's default dev branch, the diagrams have in the order of 9,000 saddle-max pairs.
when I run the same state with this PR, they have around 522,000 saddle-max pairs, which is about the number of total triangles in the triangulation (524k). I suspect something is still off.

at this point, we have two options:

  • removing the edits regarding the periodic triangulation from this PR and move forward (and carefully report the issue in the documentation)
  • try one more fix (there shouldn't be that many combinations of possible changes)

@julien-tierny julien-tierny added bug WIP work in progress and removed bug labels Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants