-
Notifications
You must be signed in to change notification settings - Fork 3
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
[VCMML-414] Flatten the variables to be coarsened #445
Conversation
The previous implementation made some assumptions about the specific budgets that prevented this script from coarse-graining additional inputs. This commit relaxes those assumptions by explicitly listing all the variables to be coarsened.
right now i am using a string, but really this should be two functions with data array arguments.
Remove mindlessly added qv_dt_nudge_coarse; update fine_res_budget.json schema. Still need to work on the attribute metadata (units, long_name etc.), but this is a start.
Ok I think this is in a stage now where it is ready to be reviewed. Based on discussions with @nbren12, I moved the eddy flux convergence portion of the calculation to within the fine-res mapper. Now the fine-res budget (maybe it should get a different name?) workflow only does the following:
Code that adds metadata to the budget terms has been moved to the fine-res mapper, while some minimal metadata is added to the second moment quantities in the budgets workflow. Due to the assumption that variables of certain names will be in the dataset opened by the fine-res mapper, there is some pretty strong coupling between the workflow and mapper portions of code; this may be unavoidable, though I'm open to suggestions to make things a little less brittle. |
#513 would be useful here too; otherwise the metadata I add prior to coarsening will get dropped. |
This will fail until #513 is 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.
I can't formally "approve" since I started this PR, but feel free to approve on my behalf.
Thanks so much for picking this up. It will be great use this code to pressure level coarse-grain many more outputs. It's possible this may cause memory problems when running in dataflow, but i don't think it is likely since I think dask operations are fully lazy.
The regression tests are currently failing on the long_name
check. I would be okay with disabling that check and merging as is, but it's up to you.
I added that knowing that #513 would provide a fix, so with that merged it passes now. |
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.
Approving on behalf of @nbren12.
The previous implementation made some assumptions about the specific
budgets that prevented this script from coarse-graining additional
inputs. This commit relaxes those assumptions by explicitly listing all
the variables to be coarsened.