Skip to content

distinguish missing from excess timesteps in timesteps validation#140

Merged
dgergel merged 3 commits intoClimateImpactLab:mainfrom
emileten:update_validation_time_check
Nov 29, 2021
Merged

distinguish missing from excess timesteps in timesteps validation#140
dgergel merged 3 commits intoClimateImpactLab:mainfrom
emileten:update_validation_time_check

Conversation

@emileten
Copy link
Contributor

@emileten emileten commented Nov 24, 2021

@dgergel this is solving #139. We're (1) raising an exception if the time span is too short, with a slightly lower expectation for 'future' 'cmip6' (2) warning the user if the time span is too long.

@brews I don't think there is a test to write, but let me know if I am wrong.

Close #139

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.

Thanks for this @emileten!

I can live with this if this fits the behavior and level of testing that @dgergel wants for validation. I'm not sure we need to check "historical" like we've done for "future" because we explicitly crop historical at 1950 -- but again, go with her feel on this.

@emileten, please also add an additional commit noting your change in HISTORY.rst. The easiest way is to copy the PR title, and note your PR number and GitHub name. We want to be sure you're credited for your contributions.

@brews brews added the bug Something isn't working label Nov 24, 2021
@brews brews requested a review from dgergel November 24, 2021 14:02
Copy link
Member

@dgergel dgergel left a comment

Choose a reason for hiding this comment

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

@emileten looks great - thanks for fixing this! Only one small requested change in the assertion statements

@emileten
Copy link
Contributor Author

Thanks both. @dgergel @brews could one of you give me write access so that I merge this branch ? Thanks !

@dgergel
Copy link
Member

dgergel commented Nov 29, 2021

Thanks @emileten! And checking on permissions for you. Merging in the meantime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

update time check in dodola validate

3 participants