Relative Economic Value implementation

[thomaspagano]

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

Development for new xarray-based metrics

• [yes ] Works with n-dimensional data and includes reduce_dims, preserve_dims, and weights args.
• [yes ] Typehints added
• [yes] Add error handling
• [yes] Imported into the API
• [yes] Works with both xr.DataArrays and xr.Datasets if possible

Docstrings

• [yes] Docstrings complete and follow Napoleon (google) style
• [yes] Maths equation added
• [yes] Reference to paper/webpage is in docstring. The preferred referencing style for journal articles is APA (7th edition)
• [yes] Code example added

Testing of new xarray-based metrics

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

Tutorial notebook

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

Documentation

• [yes] Add the score to the API documentation
• [yes] Add the score to the included list of metrics and tools

[tennlee]

Names like 'special' are a bit ambiguous - in what way are they special? Maybe something more concrete?

[climatron]

issue-632-REV afefa3a I renamed all instances of special_outputs to derived_metrics

[nicholasloveday]

I'm still working through my review, but here are some initial comments

[nicholasloveday]

Do we need to print out the xarray objects? Or is the plot enough?

[climatron]

Maybe it's overkill but I think seeing more is helpful.

I just noticed a potential issue and put it in the group chat:

a question about the "maximum" special output...

Is it right or wrong that the output should have cost_loss_ratio == threshold?

I think the optimal REV for a particular cost_loss_ratio can potentially come from a different threshold. For example... cost_loss_ratio = 0.2, REV maximum = 0.54, optimal threshold = 0.3. Is the above example misleading and if so, should the threshold coordinate be something other than equal to cost_loss_ratio. I haven't explored it, but I'm not sure if you can have repeating coordinates in xarray like 0.0, 0.1, 0.3, 0.3, 0.3, 0.5...

[nicholasloveday]

Thanks @thomaspagano - some more feedback from me

[nicholasloveday]

It's starting to get closer @thomaspagano . I've made a few more suggestions. I still need to check a couple of the tests manually.

[nikeethr]

I'm not reviewing this PR - but thanks for doing this, and I appreciate the defensive coding from a brief look.

I mostly wanted to comment about this:

try:
    import dask.array as da

    HAS_DASK = True
except ImportError:
    da = None
    HAS_DASK = False

This is not so much a "this PR" thing - and more directed at maybe @nicholasloveday and @tennlee, but is there a general utility function that can be used instead? And if not, should we be adding it?

if utils.dask_available(): do_dask_stuff or similar seems a bit cleaner to me. I notice this pattern re-emerging in other parts of the codebase and tests as well.

[nicholasloveday]

I think that the implementation works as expected, but the tutorial is wrong.

I think it could be updated along the lines of

# With check_args=True: Error not raised until compute()
try:
    result = relative_economic_value(fcst_bad, obs_good, [0.5], check_args=True)
except ValueError as e:
    print(f"error point A")

# With check_args=True: Catches the error once compute() is called
try:
    result.compute()
except ValueError as e:
    print(f"error point B")

And then it should only print ("error point B").

There's probably a better way of demonstrating it, but I think that gives you an idea of the issue.

[nicholasloveday]

Your updated version looks good. However I think that you need to re-run the notebook as the output doesn't exactly match your latest code!

[nicholasloveday]

I think this text needs updating to highlight that this only is called when the computation is triggered.

[tennlee]

This isn't wrong, but why change it? There is no logical difference, so I'm not sure why this is here.

[tennlee]

Can we get a comment on why this assertion graph is here and why it is needed?

[tennlee]

Why has this variable name changed? If it is for stylistic reasons, it should be in a separate PR since it's not specific to the REV metric. If it is for functional reasons, please let me know.

[tennlee]

Why is this here? I don't understand the functional requirement for this deferral and fine-grained dask computation management. If it is required, more comments are needed to explain it to a reader (developer) who isn't sure about the dask implementation details.

[tennlee]

This PR is very large, with interleaving of multiple concerns. As a result, it is challenging to see the core algorithm, validate its correctness, and separate that from the dask details and other stylistic changes. We may need to approach this score differently. I would propose simplifying the PR to an xarray-only implementation at first (i.e. do not attempt to support dask, throw an exception if it is detective), and remove stylistic changes to unrelated functions and tests. This will focus the first PR on the REV calculation on core supported data types and allow more straightforward validation of the correctness. Any stylistic changes across the codebase can then be dealt with separately, and the dask implementation can also then be handled separately. Merging this as-is will take extensive review and will result in the PR being deferred at least for 4-6 weeks, potentially for longer, and I may take the approach of choosing to split up the PR into iterations in any case. The core REV algorithm methods look reasonable, but in order to confirm (or disconfirm) the presence of side-effects with the dask handling methods and stylistic changes, a careful approach is needed. I hope my comments here seem appropriately constructive. I can see a lot of work went into this, but I want to propose something to simplify the review process, the development process and speed up the PR acceptance.