-
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
Adding mods to derive PRECT #260
Conversation
…am.ho files. This code could be extended to other variables.
Just to note that #240 is a similar modification to allow RESTOM to be derived. The implementation there is quite different. I'd recommend we encourage doing these kinds of simple derivations in a consistent manner. I like @bstephens82 's approach of just having a |
Ah, I had heard RESTOM might be another variable this could be applied to, but didn't realize it had already been implemented. I figured for each variable that could be derived, we would need to add another "recipe" in |
Yeah, I think adding "recipes" for each variable is the right way to go. We do use NCO to make time series files, but we have discussed that having NCO as a dependency might not be great for portability. I'm not opposed to using it in the short term, but we might need to revisit in the future. I guess my point was mostly to note that we should somehow combine this PR and #240 to allow both |
Also I believe this is mentioned (PRECT specifically) in Julie's issue #228, so hopefully this can close that issue out 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.
Thanks for working on this @bstephens82! It looks good, but I do have a few changes requests, mostly to avoid possible errors showing up if certain variables aren't present.
Derive variables acccording to steps given here. Since derivations will depend on the | ||
variable, each variable to derive will need its own set of steps below. |
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 totally fine for now, but long-term I wonder if we can figure out a way to add the derivation equations to the variable defaults YAML file itself, and then simply have this function properly parse those equations to produce the derived variable. That way future users won't have to modify the core ADF python code if they want to add a new derived variable.
Thoughts? @brianpm @justin-richling
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 that idea...I am not sure how to implement it 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.
Almost there! Just a little bit of extra clean-up on your new file-checking 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.
Super-close, just one last question.
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 now, thanks! Once @justin-richling also reviews and approves it then it will be ready to be merged.
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 looks good on my end, thanks @bstephens82! This will help get us in the right direction for more derived quantities in the future.
from PRECC+PRECL if PRECT is not in the cam.h0 files. This code could be extended to other variables.