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

Multi-dimensional example needed for crps_ensemble tutorial (and probably others) #804

Open
MischaDy opened this issue Dec 23, 2024 · 5 comments

Comments

@MischaDy
Copy link

MischaDy commented Dec 23, 2024

I had real trouble getting crps_ensemble to work for more than one observation. The outputs varied non-sensically on simple test cases where I had an idea of how they should vary, e.g
obs=[0,1] and fcst=[[-0.1, 0, 0.1], [0.9, 1, 1.1]
resulted in something else than
obs=[0,0] and fcst=[[-0.1, 0, 0.1], [-0.1, 0, 0.1].
I had to debug into the scores code to see that the issue was with my xarray dimension naming - I hadn't named the obs array's dimensions, but did name the fcst array's, just as is done in the tutorial. On this line, the subtraction fcst - obs broadcasted into a 3d-array, and that's when things went wrong. All of this is avoided if both the fcst and obs array have an identically named first (sample) dimension.
(I have never worked with xarray before, so perhaps this is obvious to more familiar users. But this shouldn't be made a tripwire for xarray noobs.)

Suggested solutions
I suggest:

  1. Appending a simple multi-dimensional example to the existing tutorial, where the users can see how an additional axis is to be handled.
  2. Raising a warning or an error when this kind of implicit broadcasting would occur. I'm sure it's fine or desired sometimes, but I doubt it's usually intentional in this case.
@tennlee
Copy link
Collaborator

tennlee commented Dec 23, 2024

Thanks very much for raising this issue - you have supplied a lot of helpful information. Thanks for taking the time to work through it and share the details. Regarding your suggested solutions, I think (1) should be done. (2) might need a bit more thinking, as there might be a lot of different cases here, as xarray broadcasting is a complicated thing and I'm not sure we can account for every situation. I'll review the code, and if there's a simple way we can add a warning message without interfering with ordinary usage, I'll happily do so. The response may take a little while to implement, as many people are on leave at the moment, but we will get to it when we can.

Also, if working with crps_ensemble, do make sure to be using at least version 1.3.0 of scores, as it contains a performance enhancement for this function, and consider upgrading to version 2.0.0.

@MischaDy
Copy link
Author

MischaDy commented Dec 23, 2024 via email

@nicholasloveday
Copy link
Collaborator

I also agree that suggestion 1 should be done.

I assumed that the forecast and observed xarray objects that were to be passed in should have all their dimensions labelled. Generally the logic in the metrics in scores works on dimension name labelling rather than axis numbers.

I can see that the dimension names weren't labelled properly in that tutorial and it should be updated to avoid confusion.

I guess there is the question of if we should explicitly check if dimensions have been labelled properly in each function or if we want to maintain the flexibility of people using unlabelled xarray objects.

@nicholasloveday
Copy link
Collaborator

I've created a pull request to update the tutorial here #805

You can preview the updated tutorial here https://github.com/nci/scores/blob/9265928946fa666fe7a2373a85a49cdcbdc26887/tutorials/CRPS_for_Ensembles.ipynb

@tennlee
Copy link
Collaborator

tennlee commented Jan 13, 2025

The PR has been merged, addressing the tutorial suggestion. I will leave this open until I can consider the second point in the next few weeks.

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

No branches or pull requests

3 participants