-
Notifications
You must be signed in to change notification settings - Fork 1
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
AGNI no-solve configuration and minor changes #334
Conversation
…strict convergence check not passed. Added wall-clock runtime to helpfile
… test the model easily even if new columns have been added.
input/demos/spider.toml
Outdated
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 no demo SPIDER file anymore?
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 created it for demo-ing the tidal heating stuff, but that works well now and can be tested through the other configs if necessary.
input/planets/k218b.toml
Outdated
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 feel we have to pick up the discussion again how to handle the config files. Since the number of parameters keeps rising, it seems to be really annoying to edit all files everytime... :> So the solution may be to instead have one "default" file and store only the parameters that are changed from the default in the specific demo cases. This will require some tweaking of how config files are read. This may actually make a good issue for @HannoSpreeuw to consider sometime in early March. What do you think @lsoucasse?
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.
Good point @timlichtenberg !
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. We decided before to not have a default one, because it could be confusing, but it feels like we have to go through and edit a bunch every time someone changes the code.
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 it should be quite easy to do though
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 the idea would be to call a specific .toml file, which only contains the parameters that are different from default.toml, correct? In this case, both the "case.toml" and "default.toml" file should be copy-pasted into the simulation directory, so that afterwards it is crystal clear which parameters were used.
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.
Yeah I think that would be a good solution. As long as it's all compiled into a file that the user can check, it should hopefully avoid issues.
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, nice job with the double check for solidification/termination @nichollsh !
@@ -107,11 +110,19 @@ def start(self, *, resume: bool = False, offline: bool = False): | |||
Run in offline mode; do not try to connect to the internet. | |||
""" | |||
|
|||
# Import | |||
from proteus.atmos_clim import RunAtmosphere |
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.
Define some common nomenclature in Docs regarding function names etc?
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.
They keep changing from CapitalLetter to under_score_naming and back.
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 code was originally written with CapitalLetter format, but we later decided that the standard should be lower_case_with_underscores. I think the latter is what Python is written in.
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.
But yes, I agree we should add this in the "contributing" section of the docs.
src/proteus/utils/terminate.py
Outdated
@@ -149,6 +149,14 @@ def check_termination(handler: Proteus) -> bool: | |||
|
|||
# Quantify model state at this iteration | |||
finished = False | |||
handler.finished2 = False |
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.
"finished2" and "finished1" are difficult to parse and understand variable names. Something more descriptive?
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'll improve this.
@@ -5,6 +5,7 @@ | |||
from PIL import Image | |||
|
|||
PROTEUS_ROOT = Path(__file__).parents[2] | |||
NEGLECT = ["CH4_mol_atm", "CH4_mol_total", "CH4_kg_atm", "CH4_kg_total", "CH4_bar", "runtime"] |
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 only these?
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.
In this PR I have just moved this functionality to a new file. We should try to keep the list of neglected keys as short as possible. CH4 is included because it's very sensitive to the numerics. Runtime is included because it depends on the performance of the computer used.
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.
A few comments that may lead to new issues.
Thanks both! I'll make some minor changes and re-request a review. |
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.
Great, improves the readability and contribution instructions a lot!
Thank you! |
atm
variable into the Proteus class.