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

Assert Counterclockwise Orientation of Read Gmsh File #115

Merged
merged 4 commits into from
Nov 14, 2024

Conversation

Fuad-HH
Copy link

@Fuad-HH Fuad-HH commented Oct 28, 2024

It is to be asserted that the triangles' nodes are given in the counter-clockwise direction in the mesh adjacency information of Omega_h mesh objects. Various computations that use Omega_h depend on this information. But when this gmsh file is read, it gives clockwise adjacency. It indicates that there is some bug in the gmsh reader.

The order has to be always (0,3,4), (3,4,0), or (4,0,3), not the other way around.

image

Copy link

@cwsmith cwsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for creating the PR. A couple comments are below.

Also, please provide a description of the PR. When this gets merged that will be the merge commit message.

src/test_tri_vert_orientation.cpp Outdated Show resolved Hide resolved
0;
ccw_face[face] = (!cw) ? 1 : -1;
};
Omega_h::parallel_for(mesh.nfaces(), find_ccw_face, "find_ccw_face");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the intention is to simply answer the question "are all faces ccw?" then this can be written as a max reduction and we can avoid storing ccw_face[face].

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this check is going into the Omega_h constructor I agree.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PUMI has a very useful 'verify' function that checks for mesh and model classification issues (I'm not sure of the full set of checks offhand). This function seems like a good first step towards an Omega_h version of that.

(faceCoords[2][1] - faceCoords[1][1]) *
(faceCoords[1][0] - faceCoords[0][0]) >
0;
ccw_face[face] = (!cw) ? 1 : -1;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why you use 1 and -1? If I ask the question is_this_counter_clockwise and you tell me -1 it seems less obvious to me than 0 which corresponds to false.

@Fuad-HH
Copy link
Author

Fuad-HH commented Oct 28, 2024

I have added the suggested changes here. I am now finding the orientation and making sure all the faces are giving the same orientation for all faces in one parallel reduction.

@cwsmith
Copy link

cwsmith commented Oct 28, 2024

@Fuad-HH Thank you.

Please provide a description of the PR in the text box that currently says " No description provided. ". When this gets merged that will be the merge commit message.

2024-10-28-162535_2367x602_scrot

@Fuad-HH
Copy link
Author

Fuad-HH commented Oct 28, 2024

@Fuad-HH Thank you.

Please provide a description of the PR in the text box that currently says " No description provided. ". When this gets merged that will be the merge commit message.

2024-10-28-162535_2367x602_scrot

It's added now.

@Fuad-HH Fuad-HH requested a review from cwsmith October 28, 2024 23:13
@cwsmith
Copy link

cwsmith commented Oct 29, 2024

@Fuad-HH Given the mesh uploaded with this PR, is the test expected to pass or fail?

Copy link

@cwsmith cwsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. There is one small edit needed.

src/CMakeLists.txt Outdated Show resolved Hide resolved
@Fuad-HH
Copy link
Author

Fuad-HH commented Oct 29, 2024

@Fuad-HH Given the mesh uploaded with this PR, is the test expected to pass or fail?

It's failing.

Copy link

@cwsmith cwsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one more change...

src/CMakeLists.txt Show resolved Hide resolved
@cwsmith
Copy link

cwsmith commented Oct 29, 2024

/runtests

Copy link

Test Result: failure (details)

Copy link

@cwsmith cwsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you.

Note, the test failures #115 (comment) were due to a problem with our self-hosted runner.

@jacobmerson jacobmerson merged commit e64bba9 into SCOREC:master Nov 14, 2024
14 checks passed
@Fuad-HH Fuad-HH deleted the gmsh_read_orient_test branch November 15, 2024 00:36
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.

3 participants