Add quantile interval score #704

reza-armuei · 2024-09-27T02:22:47Z

Please work through the following checklists. Delete anything that isn't relevant.

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

Tutorial notebook

Short introduction to why you would use that metric and what it tells you
A link to a reference
A "things to try next" section at the end
Add notebook to Tutorial_Gallery.ipynb
Optional - a detailed discussion of how the metric works at the end of the notebook

Documentation

Add the score to the API documentation
Add the score to the included list of metrics and tools

Steph-Chong

@reza-armuei - I mostly help out with documentation. Just a heads up that I often do my reviews in a few rounds, so may do another round(s) of review later.

I have made some comments about some really minor things I noticed.

I also have two questions. In included.md, you say "Quantile Interval Score" is also known as "Interval Score". Given that:

Should there be an additional entry for "Interval Score" in included.md?
Should "Interval Score" also be its own function that calls scores.continuous.quantile_interval_score (e.g. similar to some of the categorical metrics which are known by multiple names)? I don't have an opinion on this, I just wanted to raise the question for consideration.

docs/included.md

src/scores/continuous/interval_impl.py

tutorials/Quantile_Interval_Score.ipynb

tennlee · 2024-09-29T08:21:17Z

I haven’t had a chance to review your pull request in full as yet. However, in the meantime I wanted to mention a few things.

API Docstring:

Over time we have refined what we think makes a good API docstring, and have started including more things in the API docstrings.

In your API docstring, it would be great if you could please add: 

Mathjax 
An example (or examples) 
If relevant: a “See also:” section, where you mention any closely related functions

I don’t have an API docstring template (it is currently being drafted). However, here are two recent API docstrings that could be useful to look at:

https://scores.readthedocs.io/en/228-documentation-testing-branch/api.html#scores.continuous.kge

https://scores.readthedocs.io/en/latest/api.html#scores.probability.interval_tw_crps_for_ensemble (noting this one does not include mathjax)

Interval Score

I haven’t had a chance to look into this yet. But if quantile interval score is often referred to as interval score, it may well make sense to have a separate “interval score” function that calls “quantile interval score.”

However, before creating this additional function, I would suggest updating the existing docstring first. Then once that is sorted out, add the additional “interval score” function if required

DerynGriffiths

Hi @reza-armuei. I don't have the time or interest to do a full code review. I have made one suggestion for the doc string. If the code matches that in Jive then you can be pretty confident it is OK. If there are specific queries for me, feel free to contact me with them.
PS I see there is a tutorial notebook. I will have a look at that.

src/scores/continuous/interval_impl.py

tests/continuous/test_quantile_interval.py

src/scores/continuous/interval_impl.py

DerynGriffiths

I didn't find an easy way to run the notebook. The comments are all "for your consideration". Hopefully there is something useful for you.

tutorials/Quantile_Interval_Score.ipynb

src/scores/continuous/interval_impl.py

tutorials/Quantile_Interval_Score.ipynb

tennlee · 2024-10-07T05:29:35Z

@reza-armuei please add the new tutorial notebook to the tutorial gallery. I have some feedback on the notebook itself as well, but I'll do that as a PR back to your fork.

src/scores/continuous/interval_impl.py

reza-armuei · 2024-10-08T01:26:05Z

@reza-armuei please add the new tutorial notebook to the tutorial gallery. I have some feedback on the notebook itself as well, but I'll do that as a PR back to your fork.

Hi @tennlee I've already added [Tutorial](project:./tutorials/Quantile_Interval_And_Interval_Scores.md) to the included.md for interval_score and quantile_interval_score, do I need to add this tutorial to any other docs?

tennlee · 2024-10-08T01:43:08Z

@reza-armuei please add the new tutorial notebook to the tutorial gallery. I have some feedback on the notebook itself as well, but I'll do that as a PR back to your fork.

Hi @tennlee I've already added [Tutorial](project:./tutorials/Quantile_Interval_And_Interval_Scores.md) to the included.md for interval_score and quantile_interval_score, do I need to add this tutorial to any other docs?

Yes, you need to open up the Jupyter Notebook called "Tutorial Gallery" and add a link to your new notebook in there

Added quantile interval score Fixed tutorial notebook Addressed review comments Revised a test based on review comment Updated docstring Updated tutorial gallery notebook

tennlee · 2024-10-18T10:39:45Z

I have created #711 to track the goal of supporting Dataset arguments also.

reza-armuei requested review from tennlee and DerynGriffiths September 27, 2024 03:51

Steph-Chong reviewed Sep 27, 2024

View reviewed changes

docs/included.md Outdated Show resolved Hide resolved

src/scores/continuous/interval_impl.py Outdated Show resolved Hide resolved

tutorials/Quantile_Interval_Score.ipynb Outdated Show resolved Hide resolved

DerynGriffiths reviewed Sep 30, 2024

View reviewed changes

src/scores/continuous/interval_impl.py Show resolved Hide resolved