Skip to content
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

Merged
merged 23 commits into from
Jul 30, 2020

Conversation

nbren12
Copy link
Contributor

@nbren12 nbren12 commented Jul 3, 2020

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.

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.
@spencerkclark spencerkclark changed the title WIP flatten the variables to be coarsened [VCMML-414] Flatten the variables to be coarsened Jul 24, 2020
@spencerkclark
Copy link
Member

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:

  • Computes pressure-level-coarsened versions of diagnostics and restart fields, centered in the middle of the timestep.
  • Computes pressure-level-coarsened "second moment" quantities, which are products between two existing output fields, e.g. specific humidity and omega.
  • Computes the pressure-level-coarsened storage tendency terms, which require knowledge of quantities at the boundaries of timesteps, which would be awkward to compute within the mapper.

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.

@spencerkclark spencerkclark marked this pull request as ready for review July 28, 2020 20:37
@spencerkclark
Copy link
Member

#513 would be useful here too; otherwise the metadata I add prior to coarsening will get dropped.

Copy link
Contributor Author

@nbren12 nbren12 left a 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.

@spencerkclark
Copy link
Member

The regression tests are currently failing on the long_name check.

I added that knowing that #513 would provide a fix, so with that merged it passes now.

Copy link
Member

@spencerkclark spencerkclark left a 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.

@spencerkclark spencerkclark merged commit 443d9a6 into master Jul 30, 2020
@spencerkclark spencerkclark deleted the feature/coarse-grain-more-variables branch July 30, 2020 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants