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

Add MeshCAP implementation of Mesh2 to interface with Capstone #400

Merged
merged 103 commits into from
Nov 30, 2023

Conversation

bobpaw
Copy link
Collaborator

@bobpaw bobpaw commented Nov 3, 2023

Add MeshCAP implementation of Mesh2 to interface with Capstone CRE meshes.

Add Mesh2 wrapper for Capstone CRE meshes
Adds capstone_cli for test meshes
Update scorec packages to latest and add capstone requirements

Now a Mesh2 object can be made from a capstone MeshDatabaseInterface
note that capstone's MeshSmartIterator is located at the current
item, which needs to be returned before iterator_next is called.
That way MeshCAP::iterate will work as intended.
Handles seem to be unique across entities. Thus, using them to convert
M_MTopo to MeshEntity* and MeshEntity* to M_MTopo.
Since capstone does not provide attach data to mesh entities and
we had to implement tags ourselves, before destroying an element
tags must be removed manually.
this is because collapse does not work yet!
Signed-off-by: Aiden Woodruff <woodra@rpi.edu>
@bobpaw bobpaw requested a review from cwsmith November 3, 2023 15:05
@cwsmith cwsmith self-assigned this Nov 6, 2023
Copy link
Contributor

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

Looks good. Thank you.

My biggest concern is that there appears to be no tests added to ctest (git diff origin/develop | grep add_test). Before merging this we will have to add some that cover core functionality.

Some other minor edits/comments/suggestions are below.

CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
{{0,1}
,{1,2}
,{2,0}
,{0,3}
,{1,3}
,{2,3}};
#else
Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC, these changes are modifying the canonical ordering of entities. If that is correct, will enabling Capstone break support for Simmetrix meshes? If we haven't tested that combination, would you please try building with -DENABLE_SIMMETRIX=on and -DENABLE_CAPSTONE=on then running ctest?

Copy link
Collaborator Author

@bobpaw bobpaw Nov 9, 2023

Choose a reason for hiding this comment

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

Failures with Simmetrix (mpich3) (no change with nocxx11abi)

The following tests FAILED:
96 - parallel_meshgen (Failed)
97 - parallel_meshgen_surf (Failed)
98 - parallel_meshgen_vol (Failed)

Failures with Capstone

The following tests FAILED:
5 - bezierElevation (Failed)
6 - bezierMesh (Failed)
8 - bezierRefine (Failed)
9 - bezierSubdivision (Failed)
10 - bezierValidity (Failed)
75 - nedelec (Failed)
78 - h1_shape_serial (Failed)
79 - h1_shape_parallel (Failed)

Failures with Capstone and Simmetrix

The following tests FAILED:
5 - bezierElevation (Failed)
6 - bezierMesh (Failed)
8 - bezierRefine (Failed)
9 - bezierSubdivision (Failed)
10 - bezierValidity (Failed)
80 - nedelec (Failed)
83 - h1_shape_serial (Failed)
84 - h1_shape_parallel (Failed)
96 - parallel_meshgen (Failed)
97 - parallel_meshgen_surf (Failed)
98 - parallel_meshgen_vol (Failed)

My interpretation is that the combination of the two causes no extra breakage. However, the capstone failures merit investigation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for testing. I have seen the parallel_meshgen_* on the rhel9 systems (#398); as you said, they are unrelated to this PR. Ideally, we can quickly resolve the capstone test failures but I won't be opposed to merging this and creating another PR to fix the errors if time is tight/limited.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure why Morteza added this change (in commit 5def445). I need to make Capstone test meshes with tets because right now all new capstone tests pass when this section is reverted. Reverting just this section fixes bezierElevation, bezierSubdivision, nedelec, and h1_shape_*. However, it doesn't fix bezierMesh or bezierValidity which is beguiling. Is there capstone documentation that would back up the tet-reorder?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, so I've checked out the capstone mesh topologies and these are the correct values. Not sure if this will create incompatibilities.

apf_cap/apfCAP.cc Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't do an in-depth review the code added here and just marked some commented code that should be removed.

gmi_cap/gmi_cap.cc Show resolved Hide resolved
gmi_cap/libgmi_cap.pc.in Outdated Show resolved Hide resolved
apf_cap/libapf_cap.pc.in Outdated Show resolved Hide resolved
test/1d.cc Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Were there line ending changes in this file? All but a few lines are marked as edited, but they appear the same.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, there were a few lines with CRLF.

pumi/GenTag.h Outdated Show resolved Hide resolved
@onkarsahni
Copy link
Contributor

@cwsmith We have some mesh-adaptation tests based on surface meshes (in .cre format). Where and how should we add those? I assume we do not want to put them in the "core" repo. Also, note we cannot do mesh-verify on them as of now (due to being only surface mesh for a 3D domain). We do have output mesh for each case, however, a striaght comparison to output mesh is not a good idea (it will not be exact same output mesh if we were to change compilers for example) and so question is what should be 'assert' equivalent for this, compare mesh stats between reference stats and stats on adapted/output mesh, with some tolerance value.

@cwsmith
Copy link
Contributor

cwsmith commented Nov 6, 2023

@onkarsahni Sounds good.

The input .cre files should be added to the pumi-meshes repo (in an appropriately named subdir). Please create a PR for adding these.

Any check for correctness on the output (adapted mesh stats, etc.) would be fine with me.

@onkarsahni
Copy link
Contributor

@bobpaw I can think of three items: 1) add .cre meshes to repo Cameron indicated (need a PR for that), 2) add test code (e.g., analytic anisotropic sizefield) in ctest, and 3) for now add some assert statements which will look like: abs(numVerts-numVertsRef)<0.05numVertsRef and abs(numElms-numElmsRef)<0.05numElmsRef. numVertsRef and numElmsRef will be hard-coded and 0.05*numVertsRef implies different is within 5% (relative tolerance). numVerts and numElms are obtained for adapted mesh. One can also add similar assert statements with relative tolerance on mesh edge lengths, e.g., for minimum, average and maximum values.

Please proceed with these. Let me know if any questions, and keep us posted.

@bobpaw
Copy link
Collaborator Author

bobpaw commented Nov 8, 2023

@onkarsahni Sounds good.

The input .cre files should be added to the pumi-meshes repo (in an appropriately named subdir). Please create a PR for adding these.

Any check for correctness on the output (adapted mesh stats, etc.) would be fine with me.

Hi @cwsmith, I've made a local branch but need push access to pumi-meshes. Or I can make a PR from a fork.

@cwsmith
Copy link
Contributor

cwsmith commented Nov 8, 2023

@bobpaw A PR from the fork is fine. Thank you.

Remove unused files apf_capConfig.h.in, moduletemplate, gmi_cap_config.h.in
Update apf_cap/pkg_tribits.cmake
Fix unsigned integer errors in capStoneAttachSolution.cc
Remove unused .pc.in files in apf_cap, gmi_cap
Remove spurious whitespace in apfCAP.cc

Signed-off-by: Aiden Woodruff <woodra@rpi.edu>
Signed-off-by: Aiden Woodruff <woodra@rpi.edu>
Test capstone_meshes cube, cylinder, and wing with anisoadapt against reference entity counts.

Signed-off-by: Aiden Woodruff <woodra@rpi.edu>
@bobpaw
Copy link
Collaborator Author

bobpaw commented Nov 13, 2023

@cwsmith, discussed this PR with Prof. Sahni. Since all tests are passing for regular core and only a few tests are failing when ENABLE_CAPSTONE=True, I think this PR should be merged and a issues should be opened to add extra capstone test cases and sort out Bezier interaction. Moving on will give us better resources (i.e. ability to convert meshes from mds->cre) to come back and solve these issues. What are your thoughts?

@cwsmith
Copy link
Contributor

cwsmith commented Nov 15, 2023

@bobpaw That is OK with me. For the tests that pass, have they been run through valgrind to check for memory leaks or errors?

destroyNative calls gmi_destroy.
MeshCAP::~MeshCAP calls destroyNative.
This is the only resource MeshCAP manages.

Signed-off-by: Aiden Woodruff <woodra@rpi.edu>
@bobpaw bobpaw requested a review from cwsmith November 23, 2023 18:47
Copy link
Contributor

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

Looks good. Thank you.

For the record, the issue for the capstone memory leaks is here: #404

Is the .gitmodule for pumi-meshes pointing at the latest version (that has the capstone meshes)?

Signed-off-by: Aiden Woodruff <woodra@rpi.edu>
@bobpaw
Copy link
Collaborator Author

bobpaw commented Nov 30, 2023

@cwsmith it is now!

@cwsmith
Copy link
Contributor

cwsmith commented Nov 30, 2023

@bobpaw Thank you. I approved the PR. If the 'merge pull request' button isn't grayed out, please go ahead and merge when you are ready. Otherwise, let me know if you are done and I'll click merge.

@cwsmith cwsmith added the v2.2.9 label Nov 30, 2023
@cwsmith cwsmith mentioned this pull request Nov 30, 2023
@bobpaw bobpaw merged commit 25e1ae8 into develop Nov 30, 2023
4 checks passed
bobpaw added a commit that referenced this pull request May 8, 2024
We meant to do this when merging #400 but it was accidentally reverted by
  commit f42cb13.

Signed-off-by: Aiden Woodruff <aiden@aidenw.net>
@bobpaw bobpaw deleted the develop-capstone-rebase branch September 24, 2024 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants