-
Notifications
You must be signed in to change notification settings - Fork 30
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
Call MDTF from ADF, and allow list of hist_str to be processed for timeseries #293
Conversation
@nusbaume I'm trying to fix the errors/warnings but I'm confused by the very first
There is no between mine and the upstream repo.
Do I need to do something? |
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 for the new features and all the extra clean-up @bitterbark! However, I did have some questions and change requests, which would be good to resolve, or at least discuss, before this PR is merged in. Please let me know if you have any questions or concerns with any of my requests!
remove quotes Co-authored-by: Jesse Nusbaumer <[email protected]>
decapitalize Co-authored-by: Jesse Nusbaumer <[email protected]>
Jesse's suggestions Co-authored-by: Jesse Nusbaumer <[email protected]>
jesse's suggestion Co-authored-by: Jesse Nusbaumer <[email protected]>
Co-authored-by: Jesse Nusbaumer <[email protected]>
Jesse's suggestions Co-authored-by: Jesse Nusbaumer <[email protected]>
Jesse's suggestions Co-authored-by: Jesse Nusbaumer <[email protected]>
Jesse's suggestions Co-authored-by: Jesse Nusbaumer <[email protected]>
Co-authored-by: Jesse Nusbaumer <[email protected]>
Github and I did not get along very well through this process, so in order to remove the final conflicts, I had to use the web version of the file. This might have been better if it hadn't involved indentations, but I did what I could. However, I was unable to test it. (I merged the code over on my branch but again, something went wonky, so I couldn't even just throw in a new commit). So the problem is: I literally haven't been able to run this code to test it before hitting commit. Another issue: the reformatting I had done in order to pass the linting tests was sometimes thrown out in the merge. I guess Justin's code is so clean that he can get away with these problems? I am reluctant to delay getting this in any longer as re-merging can take a week of my time. Can I get a pass on those? |
Hi @bitterbark, it looks like the Github test failure is due to an actual python syntax error:
So we'll have to fix it in order for this PR to be accepted (otherwise the ADF itself would break). After that do you think this PR is ready for a re-review? If so I'll check it over and hopefully have @justin-richling run a test to make sure it runs as expected. If everything goes well we can hopefully get this PR merged in soon! |
Unfortnately the linter found an indent error. I am going back and merging by hand. Will update with a fresh pull request. |
Closing this in lieu of new PR |
This adds a section in the config and run files so the ADF can directly call the MDTF (Model Diagnostics Task Force (MDTF)-Diagnostics package) .
This is the simplest possible implementation that calls an existing, installed version of the MDTF using existing, installed conda envs. For nowl, timeseries files will be copied to the requested MDTF directory and file names. Imminent data catalogue functionality in MDTF will change this.
Basic documentation of new functionality:
in config.yaml, there is a new section:
If
mdtf_run:true
:[Detailed documentation of implementation/call tree]
(https://www2.cgd.ucar.edu/cms/bundy/Projects/diagnostics/mdtf/notes/run_mdtf_from_adf.html#Documentation)
More documentation for MDTF
hist_str
to be a listThe config.yaml file variable can now also be a list:
hist_str: [cam.h2,cam.h0]
These timeseries files are all placed in the same ts/ directory, which may need to be modified for multiple component hist strings. (The MDTF checks file types for time frequency for its own purposes).