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

Periodic mesh bugfix and test #884

Merged
merged 3 commits into from
Feb 8, 2023
Merged

Conversation

jamiebramwell
Copy link
Member

This ensures that the mesh nodal grid function uses discontinuous basis functions when the supplied mesh is periodic. It also fixes a bug that the nodal grid function could be double deleted between the mfem::Mesh and the StateManager.

…iodic meshes and avoid a double-delete between the mfem mesh and sidre
@codecov-commenter
Copy link

codecov-commenter commented Feb 7, 2023

Codecov Report

Merging #884 (f6dbd0f) into develop (015681c) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #884      +/-   ##
===========================================
+ Coverage    94.70%   94.75%   +0.05%     
===========================================
  Files          152      153       +1     
  Lines        10696    10763      +67     
===========================================
+ Hits         10130    10199      +69     
+ Misses         566      564       -2     
Impacted Files Coverage Δ
src/serac/physics/state/state_manager.cpp 93.33% <100.00%> (+0.52%) ⬆️
src/serac/physics/tests/solid_periodic.cpp 100.00% <100.00%> (ø)
src/serac/numerics/functional/functional.hpp 100.00% <0.00%> (ø)
src/serac/physics/state/finite_element_vector.cpp 77.77% <0.00%> (+2.77%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jamiebramwell jamiebramwell added ready for review Ready for active inspection by reviewers quick labels Feb 7, 2023
// indicates that the mesh is periodic and the new nodal grid function must also
// be discontinuous.
bool is_discontinuous = false;
auto nodes = pmesh->GetNodes();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't GetNodes() return NULL for other reasons (unrelated to periodicity of the mesh)?

I've asked mfem to clarify the preconditions of this function before, but it's still essentially undocumented.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. However, if it is a periodic mesh this will be non-null as it needs the discontinuous basis function. That is why I have the default of false on line 239.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I misunderstood what this was doing

@samuelpmishLLNL
Copy link
Contributor

samuelpmishLLNL commented Feb 7, 2023

Some other thoughts:

  1. I'm not convinced that periodic meshes in mfem work reliably, as discussed here: Clarify limitations of periodic meshes mfem/web#211

To summarize, even the example meshes featured on mfem website will not enforce periodicity correctly in many use cases. I think it's worth managing expectations by warning the user (if we detect a periodic mesh) that things may not work, for reasons out of our control.

  1. It's not obvious to me what mfem is doing regarding periodicity, so if we want to eventually support that as an official feature, we should do the due diligence of verifying that it works for different combinations of test/trial spaces.

@jamiebramwell
Copy link
Member Author

Some other thoughts:

1. I'm not convinced that periodic meshes in mfem work reliably, as discussed here: [Clarify limitations of periodic meshes mfem/web#211](https://github.com/mfem/web/issues/211)

To summarize, even the example meshes featured on mfem website will not enforce periodicity correctly in many use cases. I think it's worth managing expectations by warning the user (if we detect a periodic mesh) that things may not work, for reasons out of our control.

2. It's not obvious to me what mfem is doing regarding periodicity, so if we want to eventually support that as an official feature, we should do the due diligence of verifying that it works for different combinations of test/trial spaces.

Good point. I'll add a "buyer beware" warning if a user supplies a periodic mesh.

// Determine if the existing nodal grid function is discontinuous. This
// indicates that the mesh is periodic and the new nodal grid function must also
// be discontinuous.
bool is_discontinuous = false;
Copy link
Member

Choose a reason for hiding this comment

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

Can these two duplicate sections be unified into a function w/o too much effort?

@barreracruz
Copy link

I double checked in my branch with a representative test and these changes seem to fix the issues I had. Thank you!

@jamiebramwell jamiebramwell merged commit 0d463a4 into develop Feb 8, 2023
@jamiebramwell jamiebramwell deleted the bugfix/bramwell/periodic_mesh branch February 9, 2023 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
quick ready for review Ready for active inspection by reviewers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants