Conversation
8b1631a to
0e99da1
Compare
gassmoeller
left a comment
There was a problem hiding this comment.
I dont think I will be able to read the 40000 lines of code changed ;-). But what I can say: I tested this branch on my system and it works (even with some experimental changes I am making at the moment to ASPECT and deal.II). The testers also seem to pass without having to update any test output.
So I think in general there is no obstacle to the WB release and then including that in the next ASPECT release.
Is there anything specific you would like us to look at?
I think the main discussion point is that I included the LITHO1.0 dataset, so that users can directly use it. So if you can take a look at how it is included and let me know what you think, that would be great. |
0e99da1 to
70ca2ef
Compare
|
I have updated the pull request the released v1.1.0 version of the world builder. I think it should be ready to merge now. |
where do I find it? |
In include/data/LITHO1.0/ |
Why did you decide to include this as text values instead of parsing a file? If you want to include it in the source directly, wouldn't it make sense to put it into a .cc file? Hopefully, you don't include it in several translation units. :-) Does the user / ASPECT need to include it? |
I wanted to make sure it is as fast and stable as possible, so that I don't get any complains that the file could not be found if people move the gwb around. And if the reformatted dataset is stored in the repository, it might as well be directly compiled into the program so that it always just works. I don't know/think this is sustainable into the future for more/larger datasets, but I think it is worth it for this dataset specifically. We will need to think in the future how we can have a good and stable system for loading these kind of dataset (for example making a curl data loader, which can download datasets and/or having a separate repository with these kind of datasets ready for use with the world builder, which users, with a cmake variable, can automatically download and load or compile into the gwb). But this would probably also mean the world builder files will need feature flags to make sure they are reproducible (maybe let users define the dataset url in the gwb file, and only allow zenodo or similair urls?).
It is only included in parameters.cc, which is always it's own translation unit (never part of unity). So I think think this is not a problem? |
The world builder is I think ready for another release (it has been a while, there have been quite a few changes, a new world builder hack is coming up and a new aspect release is coming up), so as usual, I am making a pull request to aspect to check if everything works as expected and to get feedback if any changes need to be made for including into aspect.
Note 1: I will later manually return the
cmake_minimum_required(VERSION 3.13.4)line.Note 2: The status of the release process can be found here: GeodynamicWorldBuilder/WorldBuilder#863.