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

Added interval twCRPS for ensembles #682

Merged

Conversation

nicholasloveday
Copy link
Collaborator

@nicholasloveday nicholasloveday commented Sep 12, 2024

I have added a convenience function for calculating the twCRPS for an interval. This pull request just shows works with a single interval. The future twCRPS tutorial notebook will show how to work with multiple intervals.

Development for new xarray-based metrics

  • Works with n-dimensional data and includes reduce_dims, preserve_dims, and weights args.
  • Typehints added
  • Docstrings complete and followed Napoleon (google) style
  • Reference to paper/webpage is in docstring
  • Add error handling
  • Imported into the API

Testing of new xarray-based metrics

  • 100% unit test coverage
  • Test that metric is compatible with dask.
  • Test that metrics work with inputs that contain NaNs
  • Test that broadcasting with xarray works
  • Test both reduce and preserve dims arguments work
  • Test that errors are raised as expected
  • Test that it works with both xr.Dataarrays and xr.Datasets

Documentation

@nicholasloveday nicholasloveday changed the title added interval twCRPS for ensembles (ready for review) added interval twCRPS for ensembles Sep 12, 2024
@tennlee tennlee changed the title (ready for review) added interval twCRPS for ensembles Added interval twCRPS for ensembles Sep 14, 2024
Copy link
Collaborator

@Steph-Chong Steph-Chong left a comment

Choose a reason for hiding this comment

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

Really minor feedback re. adding page numbers to reference. That's very handy an open access article just came out!

src/scores/probability/crps_impl.py Outdated Show resolved Hide resolved
src/scores/probability/crps_impl.py Outdated Show resolved Hide resolved
src/scores/probability/crps_impl.py Outdated Show resolved Hide resolved
docs/included.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@rob-taggart rob-taggart left a comment

Choose a reason for hiding this comment

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

Hi @nicholasloveday , looks good. Just a few comments about the docstring. General comment: I think we need to keep working hard to distinguish between threshold weights and weights applied in a weighted mean of scores

cc @tennlee , so you know I've done the review

src/scores/probability/crps_impl.py Outdated Show resolved Hide resolved
src/scores/probability/crps_impl.py Outdated Show resolved Hide resolved
src/scores/probability/crps_impl.py Outdated Show resolved Hide resolved
src/scores/probability/crps_impl.py Show resolved Hide resolved
@nicholasloveday
Copy link
Collaborator Author

Thanks for the feedback @Steph-Chong and @rob-taggart. I have updated the branch with your suggestions.

@tennlee tennlee merged commit 8ff29ee into nci:develop Sep 18, 2024
11 checks passed
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.

4 participants