-
Notifications
You must be signed in to change notification settings - Fork 241
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
Improve heat flux map postprocessor and dynamic topography postprocessor #5603
Improve heat flux map postprocessor and dynamic topography postprocessor #5603
Conversation
70d2c74
to
b71e0c4
Compare
b80def3
to
e8d149d
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.
Looks good to me!
I would also be fine with these incompatible changes (yes, the output file names change, and the header changes, but I do not see how to make that backwards compatible and I think it's a clear improvement).
postprocessors now take care to not include spaces in the data file | ||
headers so that separating the header columns by spaces works correctly. | ||
<br> | ||
(Rene Gassmoeller, 2024/03/15) |
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.
(Rene Gassmoeller, 2024/03/15) | |
(Rene Gassmoeller, 2024/03/15) |
void output_to_file(const types::boundary_id boundary_id, | ||
const std::vector<std::vector<std::pair<double, double>>> &heat_flux_and_area); |
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 like how you wrote this for the heat flux; would you mind changing the variable names for the dynamic topography as well? (I looked at the .h file and was very confused about the function, because neither the documentation nor the variable names told me anything what these "values" are.)
@@ -436,7 +439,7 @@ namespace aspect | |||
} | |||
|
|||
std::string filename = this->get_output_directory() + | |||
(upper ? "dynamic_topography_surface." : "dynamic_topography_bottom.") + | |||
"dynamic_topography_" + boundary_name + "." + |
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.
What if someone hands over a boundary id that is neither top nor bottom? This is now possible and wasn't before, but I guess that actually is something we allow because we're just writing a file?
# x y dynamic_topography_bottom | ||
112500 0 -20.35932198 | ||
337500 0 -22.58806059 | ||
562500 0 -13.12252715 | ||
787500 0 55.68976686 | ||
1012500 0 55.68976686 | ||
1237500 0 -13.12252713 | ||
1462500 0 -22.58806056 | ||
1687500 0 -20.35932195 |
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 are the values different? I guess this is just floating point precision?
I addressed all comments. To your questions:
Yes, this is a case that is now supported and wasnt before. However, that should work just fine, and also this is a private function that can only be called from inside this class, so there is no danger someone calls it without being aware of this.
Yes, I think they changed some earlier time (maybe with a deal.II change), but the change is so small the tester didnt trigger. Then when I updated the results they got updated as well. |
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.
Just one more typo, otherwise this looks good! Feel free to merge yourself once you've fixed this.
Co-authored-by: Juliane Dannberg <[email protected]>
This PR contains two slightly incompatible improvements to the heat flux map postprocessor and the dynamic topography postprocessor: