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

Add global validation #118

Merged
merged 4 commits into from
Oct 20, 2021
Merged

Conversation

dgergel
Copy link
Member

@dgergel dgergel commented Oct 3, 2021

This PR adds global validation for tasmax, tasmin, dtr and pr. It supports validation of cleaned CMIP6, bias corrected and downscaled data for historical and future time periods. It includes a new service called validate with CLI interface:

dodola validate /path/to/dataset_to_validate.zarr \
--v "tasmax" 
--data "bias_corrected"
--time "future"

Validation includes the following checks for cleaned CMIP6, bias corrected and downscaled zarrs, historical and projection period:

For all variables:

  • output zarrs include the expected variable names
  • NaNs
  • output zarrs contain the correct number of timesteps

For tasmax and tasmin:

  • temperature range

For DTR:

  • DTR range
  • negative values

For precip:

  • negative values
  • maximum values are reasonable

closes #69

@dgergel dgergel added the enhancement New feature or request label Oct 3, 2021
@dgergel dgergel requested a review from brews October 3, 2021 21:46
@dgergel dgergel self-assigned this Oct 3, 2021
Copy link
Member

@brews brews left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Innnnteresting! @dgergel, I see what you're going for. I fear that this is going to read in a huge zarr store all into memory as it's written (I don't know if you've checked this) but I also think we have enough to pull out core functions and figure out a way to work around this if it ends up really being a problem.

I appreciate the unit tests!

Two small documentation things I'd recommend before you merge this.

  1. Could you update the PR description above and quickly outline what we're actually checking for the different arguments so it's clear what this new behavior is exactly. You explain the command but it would help to have a quick new-person friendly description of what's going on beyond just "validation".

  2. Also, could you please update the changelog with this addition?

👍

@dgergel
Copy link
Member Author

dgergel commented Oct 19, 2021

@brews this should be all ready - lmk if you see anything else to update before I merge!

@brews
Copy link
Member

brews commented Oct 19, 2021

Thanks, @dgergel. I think we're set to throw some data at this and take it from there. Merge away!

@dgergel dgergel merged commit 716155d into ClimateImpactLab:main Oct 20, 2021
@brews brews mentioned this pull request Oct 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add basic numerical validation?
2 participants