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

Better fail message #32

Merged
merged 7 commits into from
Dec 10, 2019
Merged

Better fail message #32

merged 7 commits into from
Dec 10, 2019

Conversation

Rlamboll
Copy link
Collaborator

Pull request

Please confirm that this pull request has done the following:

  • Tests added
  • Documentation added (where applicable)
  • Example added (either to an existing notebook or as a new notebook, where applicable)
  • Description in CHANGELOG.rst added

Adding to CHANGELOG.rst

Please add a single line in the changelog notes similar to one of the following:

- (`#XX <https://github.com/znicholls/silicone/pull/XX>`_) Added feature which does something
- (`#XX <https://github.com/znicholls/silicone/pull/XX>`_) Fixed bug identified in (`#YY <https://github.com/znicholls/silicone/issues/YY>`_)

@codecov
Copy link

codecov bot commented Dec 10, 2019

Codecov Report

Merging #32 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #32      +/-   ##
==========================================
+ Coverage   99.23%   99.24%   +<.01%     
==========================================
  Files          12       12              
  Lines         394      397       +3     
  Branches       64       65       +1     
==========================================
+ Hits          391      394       +3     
  Misses          1        1              
  Partials        2        2
Impacted Files Coverage Δ
...one/database_crunchers/quantile_rolling_windows.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad0b30b...c95e8f9. Read the comment docs.

Copy link
Collaborator

@znicholls znicholls left a comment

Choose a reason for hiding this comment

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

Why Warning rather than ValueError? An error would make more sense no? If users want to ignore it they can put it in a try, except block.

@Rlamboll
Copy link
Collaborator Author

We can do, although currently there are rather a lot of reasons why it may fail with a ValueError. This sort of warning will still need to be caught anyway. Do we want to ignore all ValueErrors from a single run of silicone in those cases? Quite possibly we do, I suppose.

@znicholls
Copy link
Collaborator

Do we want to ignore all ValueErrors from a single run of silicone in those cases?

So I think given this PR is just about what happens if there's no data, let's think about that first. As a user I'd expect an error with the message you have.

Then when we're running Silicone elsewhere (e.g. AR6 stuff) we can put the error handling in there. However in this PR I think an error is still most appropriate. If we wanted to get really fancy, we could create our own ValueError sub-class, NoData, or something.

@Rlamboll
Copy link
Collaborator Author

I've changed the error message, however I still get this weird tagging problem that means that half of the notepad checks fail. Am I just using a different package for something?

@znicholls
Copy link
Collaborator

znicholls commented Dec 10, 2019

Am I just using a different package for something?

No the order of columns in dataframes isn't stable. It's annoying but there it is. Just put # NBVAL_IGNORE_OUTPUT at the top of each of the failing cells.

@byersiiasa
Copy link

Thanks a lot @Rlamboll @znicholls - this is useful - so we will use a try/except block - but at least the warning will indicate why the particular scenario fails.

Copy link
Collaborator

@znicholls znicholls left a comment

Choose a reason for hiding this comment

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

Minor suggestion otherwise lgtm

CHANGELOG.rst Outdated Show resolved Hide resolved
Co-Authored-By: Zeb Nicholls <zebedee.nicholls@climate-energy-college.org>
@Rlamboll Rlamboll merged commit cc86a3e into master Dec 10, 2019
@Rlamboll Rlamboll deleted the better_fail_message branch December 10, 2019 10:28
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.

None yet

3 participants