-
Notifications
You must be signed in to change notification settings - Fork 239
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
Create an original.wb when running ASPECT with the World Builder #6137
Create an original.wb when running ASPECT with the World Builder #6137
Conversation
88e8a4a
to
6b4e2c8
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.
I think this is a great idea to improve reproducibility. I just have a small comment on the implementation of the copying.
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 agree with Menno and have a few more small comments.
If we were using C++17 we could use the filesystem header to copy it, but that will have to wait until we require deal.II 9.6.
6b4e2c8
to
d648dfa
Compare
d648dfa
to
7c6c9a8
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.
Thanks!
std::ifstream wb_source(world_builder_file, std::ios::binary); | ||
std::ofstream wb_destination(output_directory + "original.wb", std::ios::binary); |
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 binary? these files are text files or not?
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 don't think it really matters. My rational was (besides that was what an exampled I find used) that since we need to read it into a stream and directly out into another stream, we don't need it to be converted to text mode by the stream and it can just copy the bites. But am not sure if this is how it really works internally and I don't really think it matters.
From https://en.cppreference.com/w/cpp/io/c/FILE#Binary_and_text_modes:
A binary stream is an ordered sequence of characters that can transparently record internal data. Data read in from a binary stream always equal the data that were earlier written out to that stream, except that an implementation is allowed to append an indeterminate number of null characters to the end of the stream. A wide binary stream doesn't need to end in the initial shift state.
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 was @MFraters suggested edits so he can correct me if I'm wrong, but to my understanding setting std::ios::binary
doesn't define the file format, but that you want to copy the world_builder_file
as it exists in memory to the output file without modifying the contents of world_builder_file
.
This code does work as expected and outputs the contents of the input worldbuilder file exactly as it was input into ASPECT.
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.
Oh I didn't see that you had already replied Menno 😄
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 agree that there is no harm in doing it as binary (unmodified) data.
/rebuild |
I and others who use the Geodynamic World Builder to run all models find that we occasionally modify a worldbuilder file and/or remove a worldbuilder file accidentally. This leaves the user struggling to remember the exact details of the .wb file when trying to recreate it, and to save future headaches for all ASPECT-GWB users I think it would be very helpful to generate a file called original.wb the same way we generate an original.prm file. This PR implements this functionality!
This is likely important for @MFraters to take a look at :)