-
Notifications
You must be signed in to change notification settings - Fork 238
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
Discrete phase function #6131
Discrete phase function #6131
Conversation
Implementation Overview
TestsTest visco_plastic_phase_discreteI've added a new test called visco_plastic_phase_discrete, where I use a small lookup table to conduct the test. This lookup table includes four points, with the dominant phase index assigned a value of 0 for the two low-pressure points and a value of 1 for the high-pressure points. For the rheologic parameters, I’m using diffusion creep, assigning a weaker rheology to phase 1. The results I’ve generated include pressure (P), temperature (T), and the resulting viscosity. The change in viscosity occurs around (P = 2 \times 10^{10}), which lies between the two pressure values (0 and ( |
Test visco_plastic_phase_discrete_compI’ve also included a test case visco_plastic_phases_discrete_comp for the second composition, where I assigned a viscosity of ( Test visco_plastic_phase_discrete_comp_invertFurthermore, I've tested invert the phase lookup, such that we look for index 0 as the most abundant phase and assign it a smaller viscosity. This demonstrates the flexibility of the implementation: arbitrary phases can be assigned distinct rheology as long as they are marked in the look-up table in the first place. |
Test visco_plastic_phase_discrete_mixedThis approach is followed by another test visco_plastic_phases_discrete_mixed, assigning a 0.5, 0.5 compositional fraction to the background and the "spharz" composition to facilitate compositional mixing. The results of the mixing are shown in the attached figure. Note that the range of values differs from the previous plot. In the region of phase 0, the background composition has a viscosity of ( |
35030e0
to
1ccf79e
Compare
Some related PR ideas
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very cool, I am happy this could be implemented so cleanly / without affecting other parts of the material model. I have some suggestions on function names and code improvements though. Let me know when you have implemented them and I can take another look.
6d53ae5
to
b2f8f28
Compare
Hi @gassmoeller, Thank you for your comments to make it in better shape. I have now addressed these comments, so you can start another review. |
Could you check why the tests are failing? |
4fca07d
to
21179a1
Compare
Hi Rene (@gassmoeller ), can you take a look at the issue in "no_unity-build" test. Building on my local PC is fine. Moreover, this issue is from the reference to the functions in the newly defined PhaseFunctionDiscrete class. If it raises issue there, I am confused why the link to the previous PhaseFunction class is established in the first place. |
I'll try to get to the review tomorrow, but about the test failures: It looks like you are missing an explicit instantiation of your new template class. Check the bottom of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is much closer to being ready. I have another round of comments about documentation and some data types. I mostly want to make sure someone who is not familiar with what your new phase function does can understand how to use it from the included documentation (because even for me it is a bit hard to understand some of the details with the current documentation).
* List of phase indicators of the most dominant phases in the material data files | ||
* to construct the different phase transitions in this class |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This variable needs more explanation here and also in the declare_parameters
function, because I do not understand from the documentation:
- what does the length of the vector mean (i.e. how many entries will this vector have?)
- what does each entry of the vector mean? And why are they
double
? I would have expectedunsigned int
or maybestd::string
as entries. Indicators as in identifying a certain phase transition by number should be anunsigned int
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback! I wanted to clarify a bit about the indicator array and how it’s set up.
The main point here is to let us selectively assign different rheologies to specific phases, rather than having a unique rheology for each phase in the table. So, if the table has phases 0, 1, and 2, and we only want a distinct rheology for phase 2, we’d just include phase 2 in the indicator array in the .prm file.
I realize this setup may seem less intuitive since it might feel clearer if each phase had its own rheology. But going this route actually avoids a lot of fine-tuning in the table itself. For example, if we want a special rheology for the lower mantle, we’d otherwise need to merge those lower mantle phases into a single value. Here, we can mostly leave the table as-is and make any specific adjustments directly in the .prm.
This way, if we need to combine multiple phases (e.g., ten phases), we can do it easily in the .prm file. And if other phases just need the default olivine rheology, we can leave them out of the indicator array altogether. This keeps things flexible and allows for a range of uses.
As for reading the phases in as doubles—it’s just a practical choice since all the other columns in the table are doubles, even though that might seem a bit odd. When generating the table, it was tricky to keep one column as integers while everything else was in double format.
Hope this clears things up! Let me know if you have more questions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'll still make the index unsigned int, but I guess using a double variable in the prm file would still work if the numbers in the table are integers, it that correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Theoretically you can make this work with either double
or unsigned int
as long as you always convert correctly. But converting is always error-prone, and unsigned int
is just the more natural data type for a variable that represents an integer number (i.e. a quantity that has discrete and separate values).
I would do the following:
- use
unsigned int
for the dominant phase index and the data from the table and prm file everywhere - the only place where you have to convert from double to int should then be here, because the
StructuredData
class unfortunately interprets all the data columns as if they aredouble
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I have checked this is how the implementation currently works
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, good that it works with unsigned int
now.
Please add the following to the documentation here: For a description of the use of the phase indicators, please see the documentation of the input parameter 'Phase transition indicators' in the function declare_parameters().
383f5aa
to
6b26f58
Compare
Thanks, Rene, for a lot of insightful review points. Especially about the unique_ptr, I think this time I have a feel for that. The time I changed to unique_ptr, I had to go over a few bugs already. In a way, this forces me to be careful. Here, I leave 3 open conversations for you to respond to double-check. Let me know what you think of them. |
505381a
to
6a82e39
Compare
Is this ready for another look or do you plan to make more changes first? |
Rene, it's really for another look. Sorry, I forgot to put the comments in the end. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry it took so long to get back to this PR. This looks good, I just have 4 very minor comments left. Afterwards this can be merged. Thanks for your patience, I think this is a very nice addition to the material models.
* List of phase indicators of the most dominant phases in the material data files | ||
* to construct the different phase transitions in this class |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, good that it works with unsigned int
now.
Please add the following to the documentation here: For a description of the use of the phase indicators, please see the documentation of the input parameter 'Phase transition indicators' in the function declare_parameters().
/rebuild |
f0b6988
to
bfa3bb3
Compare
Hi Rene, I addressed these comments and rebased them into one commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thank you! Good to go.
Pull Request Checklist. Please read and check each box with an X. Delete any part not applicable. Ask on the forum if you need help with any step.
Headline
I added a new class of phase function that handles discrete phase transitions by looking up the most dominant phases in a lookup table.
This is now ready for a review. @gassmoeller technically advises me in the PR. @ryanstoner1, I borrowed some of the ideas from your previous PR. I want to thank both of your perspectives. I also want to keep @mibillen posted on the progress of this work. This is really cool idea. To those who are interested, this PR would add a flexible utility to look up phase indicators in the material data files and use a distinct set of rheologic parameters. The new class we defined here only requires a small tweak in the material model itself, and thus would be portable to material models other than the visco plastic model.
Before your first pull request:
For all pull requests:
For new features/models or changes of existing features: