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 missing in.current_cell parameter entry. #6240

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

MFraters
Copy link
Member

in.current_cell was never filled in the material_model_inputs struct, but when a cpo particle has Minerals = Olivine: Karato 2008 it is used, leading to the crasch in #6237.

This fixes #6237 for me. @danieldouglas92 could you give this a go?

@MFraters MFraters changed the title [WIP[ Fix missing in.current_cell parameter entry. [WIP] Fix missing in.current_cell parameter entry. Feb 20, 2025
@danieldouglas92
Copy link
Contributor

This does fix the test case that I posted in Issue #6237 on my local machine! Thanks @MFraters

@MFraters MFraters changed the title [WIP] Fix missing in.current_cell parameter entry. Fix missing in.current_cell parameter entry. Feb 20, 2025
@MFraters
Copy link
Member Author

Oke, it looks like this is ready for review. @danieldouglas92 will add a test based on his test setup.

@danieldouglas92
Copy link
Contributor

I created a test which imposes a differential stress of 350 MPa, and a water content which varies from 0 to 1000 along the x-dimension of the model. This means that the stress/water space follows the black line shown on the figure below. The test shows that we end up with the correct fabrics, going from an A-fabric (mineral id = 1) at x=-0.5 to a B-fabric (mineral id = 2) at x=0.5, with a transition from E-fabric (mineral id = 5) to C-fabric (mineral id = 3) in between.

karato_phase

image

Copy link
Member

@gassmoeller gassmoeller left a comment

Choose a reason for hiding this comment

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

The change looks good, can you rebase to fix the documentation tester and update the test results?

Also add a changelog entry if this was introduced before 3.0 (which I think it was), just so we know it is already fixed if the issue comes up for anyone.

@MFraters MFraters force-pushed the CPO_fix_missing_in_current_cell_parameter branch from 634ed1c to a7ce884 Compare February 25, 2025 12:24
@MFraters MFraters force-pushed the CPO_fix_missing_in_current_cell_parameter branch from a7ce884 to 9e6e137 Compare February 25, 2025 12:35
@MFraters
Copy link
Member Author

I have rebased the pull request and fixed the test results and made a changelog entry. So ready for another review.

Copy link
Member

@gassmoeller gassmoeller left a comment

Choose a reason for hiding this comment

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

Yes, good to go if you address my comments and squash your commits.

@@ -0,0 +1,5 @@
Fixed: Adds current cell to material model intpust
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Fixed: Adds current cell to material model intpust
Fixed: Add the current cell to the material model inputs

@@ -0,0 +1,5 @@
Fixed: Adds current cell to material model intpust
in the cpo particle property to fix the Olivine: Karato
2008 mineral type. Also added a test to test this feature.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
2008 mineral type. Also added a test to test this feature.
2008 mineral type. Also added a test for this feature.

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.

Crystal Preferred Orientation and Olivine: Karato 2008 Mineral Case not Working
4 participants