123 crps for ensembles #126
Conversation
rob-taggart
force-pushed
the
123-crps-for-ensembles
branch
from
1fb72ba to
c560765
Compare
There was a problem hiding this comment.
Nice work @rob-taggart . I have a few suggested changes.
I still need to work through the CRPS unit test with pen and paper to check that I get the same result.
src/scores/utils.py
Outdated
| @@ -112,6 +124,97 @@ def gather_dimensions( # pylint: disable=too-many-branches | |||
| return reduce_dims | |||
|
|
|||
|
|
|||
| def gather_dimensions2( | |||
| fcst: XarrayLike, | |||
There was a problem hiding this comment.
Add a test that checks that this works with Dataset inputs. I think it should work find with Datasets.
There was a problem hiding this comment.
So it doesn't work for datasets.
For example, if I create a dataset ds then ds.dims returns Frozen({'station': 2}).
Whereas for a data array da, da.dims returns ('station',).
See this documentation for the difference in behaviour https://docs.xarray.dev/en/stable/generated/xarray.Dataset.dims.html#xarray.Dataset.dims
For now I have changed typehints to only accept xr.DataArray.
If gather_dimensions2 is to be used universally in scores, it will have to extract the dimensions as tuples from xr.Dataset and pd.Dataframe objects too. I think that is beyond the scope of this pull request, but happy to discuss.
There was a problem hiding this comment.
FYI, I've cross-referenced this comment to the issue concerned #125
|
Can you please also add crps for ensembles to the docs |
|
@nicholasloveday , I've responded to all elements of your code review, including updating README etc. I haven't squashed commits yet |
|
Thanks @rob-taggart, all the changes look good - please tidy up the commit history. I'll let @tennlee have a look over it before it's merged. |
rob-taggart
force-pushed
the
123-crps-for-ensembles
branch
from
4c61ec9 to
2285409
Compare
|
Thanks @nicholasloveday , commits have been squashed |
|
@tennlee , waiting on your review for this before it is merged |
Nick, have you been through the tests with pen and paper as yet? |
|
Looks generally fine. Some words in the docstring for what to do with gather_dimensions and gather_dimensions2 in the meantime until suchtime as they have been integrated will be helpful. There are a few comments not responded to as yet - e.g. Nick's comment about validating the tests with pen and paper, and about adding the new methods to the docs. If those can be attended to, it should be ready to merge after that. I can see some work has been done on the docs, but because the conversations isn't marked as resolved, I'm not sure if all aspects of this have been fully completed or not. |
Yes I have done this and they look good to me. |
rob-taggart
force-pushed
the
123-crps-for-ensembles
branch
from
2285409 to
1c07c84
Compare
|
@tennlee , @nicholasloveday , I've added some comments in the |
|
@tennlee , the new tmp_coord_name is fine, just need to update the docstring. See review |
Adds
crps_for_ensembles.To do this I also wrote a second version of
gather_dimensionscalledgather_dimensions2. The plan is to merge these two versions of gather_dimensions in a later pull request and standardise the way of calling it for existing metric functions. Tidying this up will be a separate pull request.