-
Notifications
You must be signed in to change notification settings - Fork 29
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
Initial implementation of past spreading history in mass conserving slab thermal structure #654
Initial implementation of past spreading history in mass conserving slab thermal structure #654
Conversation
6066527
to
5a30e7c
Compare
{ | ||
if (plate_velocities.second.size() <= 1) | ||
plate_velocities_for_ridge.push_back(plate_velocities.first[0]); | ||
if (subducting_velocities.second.size() <= 1) |
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.
Why the subducting velocity is stored in the .first in some cases and in the .second in other cases?
Is it possible to make them always in .first or always in .second
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 agree that this is confusing and I plan on making this more consistent in the next commit!
} | ||
} | ||
|
||
const double age_at_trench = ridge_parameters[1] / spreading_velocity + trench_age_shift; // m/(m/y) = yr |
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.
My main comment is: is there a way to walk around the effective_age_shift ? this variable is not very intuitive and it takes me several times to learn its meaning.
At the same time, I am confused about why we need the time_of_ridge_migartion in the first place.
Maybe it's possible to use the subducting time and:
maybe we can define a parameter of ridge distance as:
ridge_distance = ridge_velocity * time_of_subduction
then the age_at_trench could be modified as
"age_at_trench = ridge_parameters[1] / spreading_velocity + ridge_distance / ridge_velocity
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.
The names of the variables could definitely use some improvement. But what I intend effective_age_shift
to be is a value that increases/decreases the plate age after it has been subducted, white trench_age_shift
is a value that increases/decreases what the age of the plate is at the trench, these are different variables because trench_age_shift
depends on the spreading velocity, while effective_age_shift
depends on the subduction velocity.
The idea for why I made time_of_ridge_migration
a variable is so that the user can define how long the ridge has been moving, but I'm certainly open to ideas! It's not clear to me what you mean by time_of_subduction
, though. The time that subduction started?
@@ -312,8 +345,7 @@ namespace WorldBuilder | |||
} | |||
|
|||
// Plate age increases with distance along the slab in the mantle | |||
double effective_plate_age = plate_age_sec + (distance_along_plane / plate_velocity) * seconds_in_year; // m/(m/y) = y(seconds_in_year) | |||
|
|||
double effective_plate_age = plate_age_sec + (distance_along_plane / subducting_velocity + effective_age_shift) * seconds_in_year; // m/(m/y) = y(seconds_in_year) |
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.
Isn't that just the plate_age_sec + (distance_along_plane / subducting_velocity)? So the difference to the plate_age_sec is that the material is subducted so an age is added?
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 renamed plate_velocity
to subducting_velocity
so it was clearer to me which velocity was which while writing this code, but effective_age_shift
is an additive age yes. The difference between distance_along_plane / subducting_velocity
and effective_age_shift
is that effective_age_shift
only applies over a certain length of the slab
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.
So a further confusion I notice is this variable of "plate_age_sec".
The "effective_age_shift" is added to the expression here. But the "plate_age_sec" is from the "age_at_trench * seconds_in_year." The "age_at_trench" is from "ridge_parameters[1] / spreading_velocity + trench_age_shift". So in the end, both the "trench_age_shift" and the "effective_age_shift" are added in the expression of effective_plate_age.
This I don't think make sense.
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.
Ahh yes you're definitely right. I need to be applying that to the unsubducted oceanic plate so that the ages match up, not the subducting plate!
@danieldouglas92 Thanks for the explanation, but I am still confused why the ages are modified in case distance_along_plane < (d1 - d2), what happens if distance_along_plane > (d1 - d2)? |
Nothing happens if distance_along_plane > (d1 - d2). To get something to happen to the whole slab the |
Is that because if distance_along_plane > (d1 - d2), the point wasn't returned to the surface when looking at t = t2 in the diagram? I guess I am slow to get the idea, but can you write a little more of this criteria? Thanks |
If it fits into your schedule, maybe we can meet and discuss some time tomorrow or Tuesday? I think Magali will be very busy, but I'll try to touch base with her on Tuesday afternoon. |
I guess the way that I've been thinking about it while working on this implementation is that in that schematic diagram that I drew, we have If we could meet on Wednesday that would be better for me, My schedule is completely open for Wednesday (Eastern Time)! |
dae4f4f
to
70b7076
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #654 +/- ##
==========================================
- Coverage 98.30% 98.23% -0.07%
==========================================
Files 107 107
Lines 7357 7464 +107
==========================================
+ Hits 7232 7332 +100
- Misses 125 132 +7
... and 6 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
|
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.
Nice work! And thank you @lhy11009 for your reviewing of the code :)
I have a few initial comments, which mostly stem from valgrind tester. Let me know if you want to discuss them further.
source/world_builder/features/oceanic_plate_models/temperature/half_space_model.cc
Outdated
Show resolved
Hide resolved
source/world_builder/features/oceanic_plate_models/temperature/plate_model.cc
Outdated
Show resolved
Hide resolved
source/world_builder/features/subducting_plate_models/temperature/mass_conserving.cc
Outdated
Show resolved
Hide resolved
world->parameters.coordinate_system, | ||
trench_point_natural, | ||
subducting_velocities, | ||
ridge_spreading_velocities.first); |
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.
valgrind is tripping over this line, can you check if this is actually filled?
valgrind output for gwb-dat tests/gwb-dat/cartesian_multiridge.wb tests/gwb-dat/cartesian_multiridge.dat --limit-debug-consistency-checks
...
39: 1500e3 500e3 0 10e3 1538.93 0
39: ==4763== Invalid read of size 8
39: ==4763== at 0x3C8AFC: WorldBuilder::Utilities::calculate_ridge_distance_and_spreading(std::vector<std::vector<WorldBuilder::Point<2>, std::allocator<WorldBuilder::Point<2> > >, std::allocator<std::vector<WorldBuilder::Point<2>, std::allocator<WorldBuilder::Point<2> > > > >, std::vector<std::vector<double, std::allocator<double> >, std::allocator<std::vector<double, std::allocator<double> > > >, std::unique_ptr<WorldBuilder::CoordinateSystems::Interface, std::default_delete<WorldBuilder::CoordinateSystems::Interface> > const&, WorldBuilder::Objects::NaturalCoordinate const&, std::vector<std::vector<double, std::allocator<double> >, std::allocator<std::vector<double, std::allocator<double> > > >, std::vector<double, std::allocator<double> >) (utilities.cc:1337)
39: ==4763== by 0x37232B: WorldBuilder::Features::OceanicPlateModels::Temperature::PlateModel::get_temperature(WorldBuilder::Point<3> const&, WorldBuilder::Objects::NaturalCoordinate const&, double, double, double, double, double) const (plate_model.cc:168)
39: ==4763== by 0x35DF6D: WorldBuilder::Features::OceanicPlate::properties(WorldBuilder::Point<3> const&, WorldBuilder::Objects::NaturalCoordinate const&, double, std::vector<std::array<unsigned int, 3ul>, std::allocator<std::array<unsigned int, 3ul> > > const&, double, std::vector<unsigned long, std::allocator<unsigned long> > const&, std::vector<double, std::allocator<double> >&) const (oceanic_plate.cc:176)
39: ==4763== by 0x3CE692: WorldBuilder::World::properties(std::array<double, 3ul> const&, double, std::vector<std::array<unsigned int, 3ul>, std::allocator<std::array<unsigned int, 3ul> > > const&) const (world.cc:387)
39: ==4763== by 0x31450E: main (main.cc:305)
39: ==4763== Address 0x5f82118 is 0 bytes after a block of size 8 alloc'd
39: ==4763== at 0x4849013: operator new(unsigned long) (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
39: ==4763== by 0x43341D: __gnu_cxx::new_allocator<double>::allocate(unsigned long, void const*) (new_allocator.h:127)
39: ==4763== by 0x4265D3: std::allocator_traits<std::allocator<double> >::allocate(std::allocator<double>&, unsigned long) (alloc_traits.h:464)
39: ==4763== by 0x412877: std::_Vector_base<double, std::allocator<double> >::_M_allocate(unsigned long) (stl_vector.h:346)
39: ==4763== by 0x3FFBD8: void std::vector<double, std::allocator<double> >::_M_range_initialize<double const*>(double const*, double const*, std::forward_iterator_tag) (stl_vector.h:1582)
39: ==4763== by 0x3EBB6B: std::vector<double, std::allocator<double> >::vector(std::initializer_list<double>, std::allocator<double> const&) (stl_vector.h:629)
39: ==4763== by 0x3721F9: WorldBuilder::Features::OceanicPlateModels::Temperature::PlateModel::get_temperature(WorldBuilder::Point<3> const&, WorldBuilder::Objects::NaturalCoordinate const&, double, double, double, double, double) const (plate_model.cc:168)
39: ==4763== by 0x35DF6D: WorldBuilder::Features::OceanicPlate::properties(WorldBuilder::Point<3> const&, WorldBuilder::Objects::NaturalCoordinate const&, double, std::vector<std::array<unsigned int, 3ul>, std::allocator<std::array<unsigned int, 3ul> > > const&, double, std::vector<unsigned long, std::allocator<unsigned long> > const&, std::vector<double, std::allocator<double> >&) const (oceanic_plate.cc:176)
39: ==4763== by 0x3CE692: WorldBuilder::World::properties(std::array<double, 3ul> const&, double, std::vector<std::array<unsigned int, 3ul>, std::allocator<std::array<unsigned int, 3ul> > > const&) const (world.cc:387)
39: ==4763== by 0x31450E: main (main.cc:305)
39: ==4763==
39: 1500e3 1000e3 0 10e3 500.274 0
...
source/world_builder/features/subducting_plate_models/temperature/mass_conserving.cc
Outdated
Show resolved
Hide resolved
@danieldouglas92 It's nice that you have these implemented and had a discussion with me to improve it. Previous discussion:
my WIP:
When I am done, I'll create a PR for your branch. I have some additional questions about the calculate_ridge_distance_and_spreading:
It's your call if you want to reply here or arrange another 30 min meeting. Just to save further emails: |
Hi all, I've read through the back and forth conversation here but I don't have time to review code right now, and I'd to wait until Haoyuan and Daniel are agreed that things are working. From the schematic and the demo figures it looks very promising. Key thing is to think of tests that can address "edge" cases. For example, will this work for any slab length (is there ever an issue where the slab it too long or too short?). Can you put in an assert that catches if the age at time of subduction becomes negative? This should never happen, and since this is not just an input variable now, someone could do something tricky with ridge location and spreading rate that ends up a-physical. Thank you both for working on this... its very cool feature to add. |
@mibillen Just to keep you updated. I separated the calculation of slab ages into a separate function here: |
I addressed most of the comments and added some asserts dealing with the input parameters (making sure that they are consistent with the dimensions specified in |
Once you add the assert function, I'll create a unit test to check the throw. |
Done! @lhy11009 |
@lhy11009 is there anything that I can be doing to help finish this up? I think it's just the unit_test for the age function you implemented that is left (unless I'm forgetting something else), and then I can clean up the PR and try to get this merged. |
Hi Daniel
Have you seen the new PR opened last night? It's mainly a test of the ridge
distance function in spherical.
Can you address the question I have there?
Cheers
…On Wed, Feb 28, 2024 at 8:02 AM Daniel Douglas ***@***.***> wrote:
@lhy11009 <https://github.com/lhy11009> is there anything that I can be
doing to help finish this up? I think it's just the unit_test for the age
function you implemented that is left and then I can clean up the PR and
try to get this merged.
—
Reply to this email directly, view it on GitHub
<#654 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AG2WMFOSWIT2NZB5WLYVFMDYV5IJJAVCNFSM6AAAAABDNBOHMKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRZGMYDGOJSHA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
Ph.D student of Geophysics
Earth & Planetary Sciences Dept., UC Davis
Davis, CA 95616
|
I did not see that PR thank you for directing me to it!! |
Hi Daniel As you said in the previous conversation, this is ready for a merge. It's nice we can get this done together. I'll have a discussion with Magali later about the calculate_effective_trench_and_plate_ages function and hopefully summarize it into another PR later. @MFraters What do you think? |
I changed the argument in the function |
Yeah, this should be correct. At this point, as long as all tests are passed, it's ready to be merged. |
Just another small comment. Can you add a comment on the variable of "ridge_migration_time"? Like the definition of this variable. The reason I ask is that we don't assign any time to ridge spreading, then why we need to assign a time to ridge migration? |
Cool, thanks for your work @danieldouglas92 and @lhy11009! It has been great to see you work together on this pull request :) I will try to have a look at it today. In the mean time:
|
Okay, I see, I'll deal with the test coverage in utilities.cc. @danieldouglas92 Can you deal with the other file and the typos? |
@lhy11009 I'm about to submit a change that addresses everything Menno said so I think it's all good! You don't have to worry about it but thanks |
9b0baa0
to
d479925
Compare
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.
here are already a few comments
const std::vector<std::vector<double>> &subducting_plate_velocities, | ||
const std::vector<double> &ridge_migration_times); | ||
|
||
// todo_effective |
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.
todo?
@@ -83,7 +83,7 @@ namespace WorldBuilder | |||
"in degree Kelvin for this feature. If the model has an adiabatic gradient" | |||
"this should be the mantle potential temperature, and T = Tad + Thalf. "); | |||
|
|||
prm.declare_entry("spreading velocity", Types::OneOf(Types::Double(0.01),Types::Array(Types::ValueAtPoints(0.01, std::numeric_limits<uint64_t>::max()))), | |||
prm.declare_entry("spreading velocity", Types::OneOf(Types::Double(0.05),Types::Array(Types::ValueAtPoints(0.05, std::numeric_limits<uint64_t>::max()))), |
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.
Why did you change the default from 0.01 to 0.05?
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 just thought that 1 cm/yr seemed to slow and 5 cm/yr made more sense!
double spreading_velocity_at_ridge_pt = spreading_velocity_at_ridge_pt1; | ||
double subducting_velocity_at_trench_pt = subducting_velocity_at_trench_pt1; | ||
|
||
if (compare_distance2 < compare_distance1) |
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.
Could you add a comment somewhere what distance1 and distance2 represent, or make the name more clear? Secondly, it doesn't seem to be used in any of the tests, so could you add a comment on in what cases this is used, and/or add a testcase where it is used?
@danieldouglas92 nice job. Magali and I just touched base on what the effective age and the age_at_trench mean in the mass conserving model. The effective age could be calculated by the total distance of the query point on the slab to the current ridge, while the age at the trench was the trench age when this query point is at the trench (purple Age_tr_1 in this case) and it could be derived by subtracting the time of subduction. (d_plane / Vsubduction) if the subduction rate doesn't change. The idea of following the query point for it's current effective age and past trench age is to conserve energy while subducting back in time. The figure shows a case with no ridge migration but with trench retreat motion (Vsubduction > Vspreading). So it's straightforward to compute these two. Moreover, I think to move forward with ridge migration, the approach is to propagate that effect into the subducting velocity and create a function to calculate t_subduction (time of subduction of the query point). The current implementation is good in that we match the previous test. But I'll soon make an update to address this in the following PR. Daniel, can you go to the function of calculate_effective_trench_and_plate_ages and remove the two time shifts at this point? Such that we are dealing with only a trivial case at this point? Let me know what you think. Should we touch base on this as well? I can meet Fri between 11:00 am - 14:00 pm, Eastern time |
Yes we can call tomorrow at 11:00am EST! I think it would be good to discuss what you mean because as far as I can tell what you say is what is currently implemented? The effective age is being set based on the distance that a point has moved (d_plane in your diagram) relative to the ridge but is scaled based on the subducting velocity. I think the trench_age_shift should be removed yes but the effective_age_shift seems correct to me. Also, trench_age_shift is currently multiplied by 0 anyway so it's effectively pointless having it in the PR anyway. |
Just to clarify my previous comment, this is how we calculate the effective age shift, which is equal to what you call t_subducting in your diagram, and this is added to |
08b7abb
to
e5506c7
Compare
Hi Daniel, see you tomorrow at 11:00 EST ! we are yet to come to the same page with ridge migration and this sign_v multiplier. I agree the distance of ridge migration would affect the total distance of subduction. But wouldn't that be handled in the subducting velocity? Can't we just use that value? Also, are you preparing to return different values of ridge drift for different query points on the slab? Like, if the ridge starts to move at 0 Ma, and subduction also initiates, then at 10 Ma, a deeper point would have a larger ridge migration time than the shallower point. See this formulation
A naive way to think is the ridge_drift distance must be different cause it divides the distance_along_plane so they should both vary with the position of the query point. No worry, it wouldn't require much effort to address these questions and move forward. And before, I haven't asked these questions. And I think these are not what we need to address anyway in this PR. |
For your example of ridge migration and subduction both beginning at 0 ma, the current formulation does not increase the age more down dip, it does the opposite. In fact, the largest shift happens at the trench, and when distance_along_plane > ridge_drift_distance there is no age shift at all. Anyway, it will be good to talk about this to tomorrow because i'm sure the problem is actually easy to figure out if we discuss it for a few minutes. |
@lhy11009 I actually sat down and thought about what the implementation is actually doing and you're right that the implementation is definitely not doing what I thought it was doing! Looking forward to hearing your plan tomorrow |
5f33895
to
581e960
Compare
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.
Based on the discussion this looks good to me. @lhy11009, can you confirm that this what you and @danieldouglas92 discussed, and if so approve the pull request?
CHANGELOG.md
Outdated
@@ -11,6 +11,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm | |||
|
|||
## [Unreleased] | |||
### Added | |||
- Implemented a method for modifying the mass conserving temperature model based on the movement of a spreading center through time. \[Daniel Douglas; Haoyuan Li; 2024-02-29; [#654](https://github.com/GeodynamicWorldBuilder/WorldBuilder/pull/654)\] |
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.
please update this
looks good to me |
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.
nice work
581e960
to
16a77d8
Compare
16a77d8
to
3a5fa35
Compare
Thanks again for your work on this! |
Began an implementation into the oceanic plate and subducting plate thermal plugins, in response to #529. The idea is that over time spreading ridges are not stationary, and as they move closer/further from the trench the age of the plate entering the trench decreases/increases. This impacts what the state of the thermal of the slab is. I have an initial implementation that is not done, but has promising results. I've attached an image that shows an example where the ridge has moved farther from the trench, increasing the age of the plate as it subducts for 20 Myr. The first shows an example where the ridge does not move relative to the trench, and the image shows isotherms for cold slab temperatures. The second image shows the case where the ridge has moved closer to the trench, resulting in a younger, hotter slab at the trench, and the third image shows the reverse case where the ridge has moved farther from the trench resulting in a colder, older slab at the trench. I've also attached an example .wb file.
This needs to be extended to account for the temperature in the unsubducted oceanic lithosphere as well, because as is evident in the pictures there is a clear discontinuity at the trench, but I think this is close to being completed, maybe 60-70% done.
ridge_drift.txt