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.

Sign up for GitHub

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

Jump to bottom

Validation fixes #120

Merged
merged 11 commits into from
Oct 25, 2021
Merged

Validation fixes #120

merged 11 commits into from
Oct 25, 2021

Conversation

brews
Copy link
Member

@brews brews commented Oct 22, 2021

Several small bug fixes, doc improvements, and test fixes for dodola validate-dataset.

brews added 9 commits October 22, 2021 15:27
@brews brews added bug Something isn't working documentation Improvements or additions to documentation labels Oct 22, 2021
@brews brews self-assigned this Oct 22, 2021
@brews
Copy link
Member Author

brews commented Oct 22, 2021 •
edited
Loading

@dgergel I had time to fix up features added from PR #118. I think I got the outstanding issues but I could use your help with the last issue, if you would.

As it was written, the unit test for #118 had a couple of problems. First it actually would have tested the imported dodola.core.validate_dataset instead of dodola.services.validate. However, the test was also missing the test_* prefix in its name and thus never actually ran. I fixed up the test and its parameterizing to what I think you were going for. It currently runs and fails when testing the number of time steps in the test data (etc). Would you take a look at this PR and ensure this test (and validation) runs and passes with your desired behavior?

@brews brews assigned dgergel and unassigned brews Oct 22, 2021
@dgergel
Copy link
Member

dgergel commented Oct 25, 2021

@brews I just fixed up the tests so they should be good to go now. Thanks for your help with this!

@brews brews marked this pull request as ready for review October 25, 2021 22:02
@brews brews requested a review from dgergel October 25, 2021 22:02
@brews
Copy link
Member Author

brews commented Oct 25, 2021 •
edited
Loading

@dgergel Thanks. If it looks like this PR keeps with your original's PRs intentions for validation, then I think we can merge this. 👍

dgergel
dgergel approved these changes Oct 25, 2021
View reviewed changes
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.

I think this looks great and I don't see anything that changes the desired behavior. Thanks again for your help with this @brews!

@brews brews merged commit 38312fe into ClimateImpactLab:main Oct 25, 2021
@brews brews deleted the validation_fixes branch October 25, 2021 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dgergel dgergel dgergel approved these changes

Assignees

@dgergel dgergel

Labels
bug Something isn't working documentation Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@brews @dgergel