-
Notifications
You must be signed in to change notification settings - Fork 33
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
adjust function Utilities::calculate_effective_trench_and_plate_ages #694
adjust function Utilities::calculate_effective_trench_and_plate_ages #694
Conversation
|
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.
@lhy11009 This looks good! So this is from the perspective of the trench, and if subducting_velocity
> spreading_velocity
the trench advances and age_at_trench
increases, and if subducting_velocity
< spreading_velocity
, the trench retreats and age_at_trench
decreases. I think this makes sense, let me know if I can help with anything but I like what you've done here!
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 working on this @lhy11009!
I liked that you used unit tests to test the functions. In this case, I would also like to see some test where is is used in an input file, so we can make sure the whole system works as intended. Can you also add some tests for this (preferably .dat tests)?
Your new assert seems to be triggered in some of the existing test, could you have a look at that as well?
have this issue I want to assert the subducting velocity as positive in the code
And then some errors are triggered. Then I realized the default value is -1.0 indicating we want to use the spreading velocity instead. Is there a workaround where we can still check if a incorrect negative value is entered versus if we want to use the spreading velocity instead? Some trials: I tried to initiate the subducting velocity as nan value, but there is a check for the input values. So this would trigger another error.
@MFraters ? Any idea. |
Thinking about this, the ideal case would be that you use an WBAssert, not WBAssertThrow here. The real check (WBAssertThrow) should be in parse parameters. In there you want to convert the value to the actual value over there. Do you think it is feasible to do something like that here? |
@MFraters, thanks for your anwser but not sure if I get the idea. Are you saying inserting an assert into the parse parameters to check if subducting velocity is positive instead of checking for it in the function? |
What if you change the default value to 0? Require that if subducting velocity = 0 to use the spreading velocity, and throw an error if it’s negative? |
|
5bb5699
to
a3624b5
Compare
I have no problme running the gwb-dat command with
but running gwd-grid with command would produce an error
|
@MFraters do you have an idea why this happens? |
I assume that can you post the gdb backtrace and the valgrind output? |
Oh, I happened to use the dat file instead of the grid file, my bad. |
source/world_builder/features/subducting_plate_models/temperature/mass_conserving.cc
Outdated
Show resolved
Hide resolved
Another note here, I mean to change the variable "plate velocity" to "spreading_velocity" or "ridge_spreading_velocity". I saw Daniel already call it this way in pieces of comments and inside other functions. I'd like to know what do you think? |
@lhy11009 I think it makes sense to distinguish the velocities more clearly by using "spreading velocity", this would also break all other previous input files, but since you're making "subducting velocity" a required parameter I guess it better to break multiple things at once! Thoughts on this @MFraters? |
Yes, this would break input files even more, but in this case I think it makes sense. It is best to get the names sorted out now properly, before the 1.0 release. Feel free to make this change. |
@lhy11009 Thanks for posting the pictures, they look cool! The bottom seems to be quite sharp, is that because it is cut off? I am not sure I follow what is happening with the slab which is both warmer and cooler, but I will have to think a bit more about that :) |
@MFraters , that's a good point and I mean to write more about the temperature differences. In the case of trench retreat, an older age in the past addressed the colder temperature in the slab's internal. For the mantle above the slab, the warmer temperature is from the heat conduction. Specifically, the subducting velocity increases from 5 cm/yr in the reference case to 10 cm/yr here. So the effect is, that the transit time of the slab is decreased by half everywhere. Thus the slab has only half the time to cool the mantle above resulting in a warmer temperature in the overlying mantle. I actually have a question myself regarding the advance case, I think it should be opposite to the retreat case. |
Addressed tasks:
Any other thoughts? |
9bc4197
to
8356180
Compare
@lhy11009 I think to isolate the effect of trench advancement/retreat you could make the subducting velocity the same in both cases, but make the spreading velocity different. This will make the heat conduction the same, and if you make it so that the age of the plate at the trench is the same in both cases (by moving the location of the ridge based on the spreading rate), this would effectively create a case where you are only capturing the temperature difference based on trench retreat/trench advancement. |
source/world_builder/features/subducting_plate_models/temperature/mass_conserving.cc
Outdated
Show resolved
Hide resolved
Good point, I'll generate these new tests. |
408a802
to
fbbda22
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #694 +/- ##
==========================================
+ Coverage 98.19% 98.21% +0.02%
==========================================
Files 107 107
Lines 7464 7462 -2
==========================================
Hits 7329 7329
+ Misses 135 133 -2
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Very cool!!! |
I see there is some coverage issue. I will fix this shortly |
ccea22b
to
345a3d2
Compare
@MFraters, I have fixed two spots of the code coverage issue. But there is still one in parameters.cc. I haven't changed any code there, can you take a look at why that code coverage decreases? |
@MFraters Can you take a look at this again ? |
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.
I am sorry it took so long for me to take a look at it. The code looks great to me, just a few small comments and questions for clarification. @danieldouglas92, could you approve this pull request as well if you think it it ready to merge?
The piece of code in paramters.cc you are refering to is filling in the default value if needed. It might be that you added a value in one of the tests, which first would have taking a default value. Don't worry to much about it for now.
@@ -42,7 +42,8 @@ | |||
"temperature models":[{"model":"mass conserving", | |||
"reference model name": "plate model", | |||
"density":3300, "thermal conductivity":3.3,"adiabatic heating":false, | |||
"plate velocity":0.03, | |||
"spreading velocity":0.03, | |||
"subducting velocity":0.03, |
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.
can you align this?
cookbooks/simple_subduction_2d_chunk/simple_subduction_2d_chunk.wb
Outdated
Show resolved
Hide resolved
source/world_builder/features/subducting_plate_models/temperature/mass_conserving.cc
Show resolved
Hide resolved
345a3d2
to
14d9cfd
Compare
@MFraters I addressed all your comments. |
Why is the subducting velocity sticking out so strangely in the .wb file compared to the other lines? |
It's fine on my computer. I think there is a problem converting \t to " ". For some reason these files are not covered by astyle. |
14d9cfd
to
33c703e
Compare
Looks good to me! Super exciting that this is a feature now |
yes, the .wb files are not processed by astyle. I didn't think it would make sense to do that, but maybe it would be useful. Anyway thanks for your work on this! |
I have added a few tests in this PR. You can scroll upward and figure it out from the prm and picture. |
@danieldouglas92 , @MFraters Address what left in PR #654
mainly adust the function of Utilities::calculate_effective_trench_and_plate_ages
also added a few tests