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

Parallel Unstructured Mesh Infrastructure (PUMI): Attaching Gmsh Physical Entity Tags [cws/pumi-gmsh-phys-ents] #2782

Merged
merged 10 commits into from
Feb 8, 2022

Conversation

cwsmith
Copy link
Contributor

@cwsmith cwsmith commented Jan 24, 2022

This PR adds support for attaching Gmsh physical entity tags stored in PUMI meshes as an attribute on an MFEM mesh. See issue #2726 for development and testing discussion.

This PR is backward compatible with PUMI meshes created with PUMI <= 2.2.6, but only PUMI meshes created with the upcoming PUMI 2.2.7 release or the current master@19185e9 branch will contain the Gmsh physical entity tags.

PR Author Editor Reviewers Assignment Approval Merge
#2782 @cwsmith @tzanio @tzanio + @mlstowell 01/27/22 02/01/22 02/08/22

@cwsmith cwsmith added the mesh label Jan 24, 2022
@cwsmith cwsmith self-assigned this Jan 24, 2022
@tzanio
Copy link
Member

tzanio commented Jan 25, 2022

@cwsmith, please mark as ready-for-review when this is ready.

(see https://github.com/mfem/mfem/blob/master/CONTRIBUTING.md#quick-summary)

@tzanio
Copy link
Member

tzanio commented Jan 27, 2022

This PR is now under review (see the table in the PR description). To help with the review process, please do not force push to the branch.

Copy link
Member

@tzanio tzanio left a comment

Choose a reason for hiding this comment

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

Thanks @cwsmith !

@tzanio tzanio added this to the mfem-4.4 milestone Jan 27, 2022
@tzanio tzanio added the in-next label Feb 1, 2022
@tzanio
Copy link
Member

tzanio commented Feb 1, 2022

Merged in next for testing...

@tzanio
Copy link
Member

tzanio commented Feb 1, 2022

Can you fix the style please: https://github.com/mfem/mfem/runs/5023713810?check_suite_focus=true#step:4

(the code-style CI check above didn't run because the PR is from a fork)

@cwsmith
Copy link
Contributor Author

cwsmith commented Feb 1, 2022

@tzanio The style should be fixed with 69c9160. I'll be sure to create branches in the mfem repo for future PRs.

@tzanio
Copy link
Member

tzanio commented Feb 1, 2022

Thanks @cwsmith !

@v-dobrev
Copy link
Member

v-dobrev commented Feb 2, 2022

Hi @cwsmith, our nighly tests uncovered a build issue with PUMI v2.2.6:

ex1p.cpp:152:29: error: invalid conversion from 'const ma::Input*' to 'ma::Input*' [-fpermissive]
          crv::adapt(uniInput);
                             ^

Is it possible to change the PR to keep it compatible with PUMI v2.2.6, or should we consider PUMI v2.2.7 to be the new minimum version requirement for PUMI? Maybe we can use auto when defining uniInput? I'm not sure about the other code changes.

@cwsmith
Copy link
Contributor Author

cwsmith commented Feb 2, 2022

@v-dobrev Commit a88269f using auto was just pushed. Thanks for the suggestion. This should restore compatibility with PUMI 2.2.6.

I'm fine with supporting a small range of minor PUMI versions or increasing the minimum required version. PUMI 2.2.7 should be released in late February (... after SIAM PP22). Do you have a preference?

@v-dobrev
Copy link
Member

v-dobrev commented Feb 4, 2022

Thanks, @cwsmith!

Let's keep the required PUMI version at 2.2.6 since 2.2.7 is not released yet.

Actually, right now the required version is listed as PUMI == 2.2.3:

mfem/INSTALL

Line 707 in a96065e

Versions: PUMI == 2.2.3.
We should update this accordingly: either PUMI >= 2.2.3, PUMI >= 2.2.6, or something like that.

@cwsmith
Copy link
Contributor Author

cwsmith commented Feb 4, 2022

@v-dobrev You're welcome. The minimum pumi version listed in the INSTALL file was increased to 2.2.6 in fb41c29 and 606ba7c increased the minimum version in CMakeLists.txt.

I didn't see a version check mechanism in the makefile based build system. Did I miss it (I didn't dig too deep)?

@v-dobrev
Copy link
Member

v-dobrev commented Feb 5, 2022

I didn't see a version check mechanism in the makefile based build system. Did I miss it (I didn't dig too deep)?

No, there is no version check in the GNU make build system.

Also, there were no issues in the nightly tests with the latest commit, so this should be ready to be merged.

@tzanio tzanio merged commit 39b521e into mfem:master Feb 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants