GMG: support prescribed values in parts of domain #6731
Conversation
|
@mibillen Would you be able to try out this PR with the different test cases you ran? Thank you! |
|
(this only works for velocity constraints and so |
tjhei
force-pushed
the
gmg-prescribed-velocity
branch
from
9e9d970 to
da5c980
Compare
|
Hi Timo, Sorry for the delay, I was traveling. I've tried testing this version but the velocity is still not applied to the solution (see attached image) Can you confirm that my steps for downloading your PR are correct: 
|
|
Yes, that should work. Is this using the old technique (signal) like the test or the new way? |
|
I used the new way that Haoyuan and Rene implemented. But since you asked, I just ran using the old way (with the separate shard library) and that one also does not work. I've attached both of the parameter files. |
|
This is what I get with my patch for your first .prm file: Without the patch, I only get large velocities on the diagonal. |
|
Yes this is correct – I was worried that I didn’t download/compile the pr correctly which is why I sent the steps I did.
I haven’t had to download from a PR before. Still not sure why this didn’t work on my end, but glad that it is working.
Thank you for fixing this issue so quickly.
Magali Billen
Professor and Department Chair
Earth & Planetary Sciences
UC Davis
On 10/29/25, 7:24 AM, "Timo Heister" ***@***.******@***.***>> wrote:
[https://avatars.githubusercontent.com/u/1531285?s=20&v=4]tjhei left a comment (geodynamics/aspect#6731)<#6731 (comment)>
This is what I get with my patch for your first .prm file:
image.png (view on web)<https://github.com/user-attachments/assets/36858ab7-7ab4-4ebe-b1c5-6df15814a99b>
Without the patch, I only get large velocities on the diagonal.
—
Reply to this email directly, view it on GitHub<#6731 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACS64URAVZJYYIPJNOFVQ4332DEZLAVCNFSM6AAAAACKDXRXOWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTINRRHAZDCMJWGM>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
There was a problem hiding this comment.
Thanks this looks like the right fix! However, there are two things I dont understand (see comments), I would like to understand them before merging this, just to make sure there is no other bug hiding in there. Do you have an idea? If not I can also try to investigate later.
| sim.prescribed_solution_manager.constrain_solution(constraints_v); | ||
|
|
||
| // Let plugins add more constraints if they so choose: | ||
| sim.signals.post_constraints_creation(*this, constraints_v); |
There was a problem hiding this comment.
Oh, so we had a bug here? Is that why the prescribed_velocity test and the other signal tests need an update even though they dont use the prescribed solution manager?
There was a problem hiding this comment.
both were missing: prescribed_solution_manager and the signal (the old way of adding constraints)
There was a problem hiding this comment.
I dont understand why only this test is affected, even though the other three prescribed_solution_velocity_function_... tests have the same settings? And in #6660 we tested that these tests give the expected results, which they did. Do you understand the difference between the tests?
There was a problem hiding this comment.
Oh, I think I at least understand the second part. Looking at this again: #6660 (comment) We likely had this problem from the beginning that only the velocities inside the indicated region were prescribed and velocities outside just wouldnt react to them. However, that still doesnt explain why the other 3 tests are not affected by this change. Do the tests maybe not see/expect velocities outside the prescribed region?
There was a problem hiding this comment.
Weird, yes. It could be that the output we print doesn't show the change?
There was a problem hiding this comment.
oh, the tests work fine if the values are constrained to velocity 0.
|
@gassmoeller The tests produce output at t=0 and t=1 year while the constrained values are linear in t. Without this PR they constrained values are set to zero velocity, which is quite close to the expected values. |
|
Ah, I see, thanks for checking. Is there an easy way to modify the tests to make them more sensitive? Increase the velocity or start with a non zero velocity? Or do you think it isnt worth the effort? |
I added a commit that changes the constrained velocities for the three unaffected tests. If the tests fail now, I can open a separate PR to change them before we merge this PR. |
|
Let's merge #6743 first and then rebase this PR to confirm things are tested sufficiently. |
tjhei
force-pushed
the
gmg-prescribed-velocity
branch
from
d8a122a to
d5dc086
Compare
|
Hm. The tests are not sensitive enough it seems. I will need to set things up locally and confirm first. |
3 participants
This might be the fix for #6726