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 hypoelastic instability #773

Merged
merged 3 commits into from
Dec 26, 2024

Conversation

ChrisZYJ
Copy link
Contributor

@ChrisZYJ ChrisZYJ commented Dec 24, 2024

Description

Fixes #771 by reverting the hypoelasticity changes from #727
Adds a test case to prevent similar issues in the future
Also regenerates the hypoelasticity-related test that was generated during #727

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Scope

  • This PR comprises a set of related changes with a common goal

How Has This Been Tested?

A new test case has been added that covers a scenario similar to #771 and is more comprehensive than existing tests.

The test was generated using commit 108805c (before #727), fails after the changes in #727, and passes after the changes in this PR.

Copy link

codecov bot commented Dec 24, 2024

Codecov Report

Attention: Patch coverage is 5.45455% with 52 lines in your changes missing coverage. Please review.

Project coverage is 43.72%. Comparing base (06f4418) to head (3779fe3).
Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
src/simulation/m_hypoelastic.fpp 5.45% 51 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #773      +/-   ##
==========================================
- Coverage   43.80%   43.72%   -0.09%     
==========================================
  Files          65       65              
  Lines       19011    19052      +41     
  Branches     2313     2318       +5     
==========================================
+ Hits         8328     8330       +2     
- Misses       9276     9320      +44     
+ Partials     1407     1402       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sbryngelson
Copy link
Member

@ChrisZYJ I understand why you want to do this. Part of the merged PR #727 was to set the 'groundwork' for pending PR #767 on hyperelasticity and RMT. So, I'm hesitant to merge this because it could mess up the work in #767. Can you work with @mrodrig6 to find a reasonable solution to everything and both PRs?

@ChrisZYJ
Copy link
Contributor Author

@sbryngelson Thanks for raising this! I’ve double-checked, and #767 doesn’t touch m_hypoelastic.fpp, so reverting the hypoelasticity changes shouldn’t conflict with the hyperelasticity/RMT updates. By fixing the stability issue now and adding a test to catch similar regressions, I think #767 would have a better 'groundwork'.

@mrodrig6 Could you please confirm there’s no hidden overlap we’re missing? Thanks!

@mrodrig6
Copy link
Member

mrodrig6 commented Dec 26, 2024

@sbryngelson Thanks for raising this! I’ve double-checked, and #767 doesn’t touch m_hypoelastic.fpp, so reverting the hypoelasticity changes shouldn’t conflict with the hyperelasticity/RMT updates. By fixing the stability issue now and adding a test to catch similar regressions, I think #767 would have a better 'groundwork'.

@mrodrig6 Could you please confirm there’s no hidden overlap we’re missing? Thanks!

@sbryngelson @ChrisZYJ Reverting would undo the central finite differences for grid stretching that were implemented in the current RMT/hyperelastic commit. The previous commit (commit https://github.com/MFlowCode/MFC/commit/108805c71480160121bd0b722d7138268ee891a6) uses the hardcoded uniform finite difference approach.

Two possible sources of issues: 1. advection and 2. RHS m_hypoelastic. If it is the RHS, it would be the finite-difference stencils in m_helpers as this was the only thing that was changed. In m_riemann_solvers a few things were streamlined for HLLC, but HLL was not touched when interacting with hypo

@ChrisZYJ
Copy link
Contributor Author

ChrisZYJ commented Dec 26, 2024

@mrodrig6 Thank you very much for clarifying! The issue was completely resolved by reverting only m_hypoelastic, likely due to finite-difference stencils in m_helpers.

I agree that grid-stretching is an important feature to keep. May I know if you plan to address the stability issue while preserving grid-stretching in your ongoing commit #767? If so, I suggest merging this #773 first, as its test suites provide correct reference results for easier debugging. You can maintain the existing m_hypoelastic code in #767, and the tests will pass once the issue is fixed.

Alternatively, if #767 is complete and you prefer to look into this issue in a separate PR, I can rebase this PR on your completed #767.

Please let me know if you have any other concerns, and I'm happy to help!

@sbryngelson
Copy link
Member

@ChrisZYJ I think we need stability before grid stretching, so I lean towards merging your PR #773 and then having you perhaps help @mrodrig6 get the right FD stencils in #767?

@ChrisZYJ
Copy link
Contributor Author

I'm not too familiar with the new FD stencil implementations, but I can look into them!

@sbryngelson sbryngelson merged commit 630695c into MFlowCode:master Dec 26, 2024
28 checks passed
sbryngelson added a commit that referenced this pull request Dec 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Instability in Hypoelastic Simulations
3 participants