precip units temporary replacement

[emileten]

This solves #125, (precip units incompatible with the xclim pint registry)

1. added an units_replacement parameter to each services.apply-* and services.train-* function, to (optionally) temporarily swap the variable units attribute for a chosen string, during bias correction and downscaling.

2. this option is used in each upstream cli function if variable=='pr' to temporarily swap units with "mm/day".

@brews (1) is general (variable agnostic) in case we want to make use of that in other cases, but (2) doesn't give an option and uses it for precip -- so that we don't need to change workflow templates now. I didn't write a unit test, it sounded like a lot of code to me for what it is. I am not fully aware of our test coverage ambitions though. Let me know.

[brews]

I think this is a good start @emileten. I have a couple suggestions and required changes.

• First and most important change. We need to test this new behavior to ensure we're getting what we want both now and into the future -- I think this is especially important here because we're depending heavily on the behavior in an upstream package (xclim and pint), so if they change things in their code, we want our tests to detect whether the behavior in our services changes. I think we're are going to want tests for each of these dodola.services.train-* and dodola.services.apply-*. We need to show that the services run successfully with the behavior described in update precip units in cmip6 cleanup to be pint-compatible #125 with the changes from this PR. I'd use data with units "mm day -1 " in the test case. The data can be made up synthetic data, or derived from the existing service tests. But the test should fail before this PR's changes and then pass after this PR's changes.

• In dodola.cli, you set the units_replacement without user input. I think I see what you're going for, but, the logic doesn't really involve the CLI -- this is about how we're handling data and downscaling biascorrecting stuff so it needs to go to the dodola.services we're using or a new function or object in dodola.core. The logic definitely needs to get moved out of dodola.cli. The second thing is that I'm a little nervous about doing this unit replacement automatically because it feels very magical, rather than explicit. If you want to keep the magic, I'd check that the input units are what we assume they are before they get replaced. Maybe log a message saying that the replacement is happening.

[emileten]

@brews thanks for the thorough review 👍 have it first on my to-do list.

[emileten]

@dgergel in what context did you encounter the bug that made you file this issue in the first place ?

I addressed brewster's requests with a much safer approach, that is using xclim's baked-in CF conversion to their pint unit registry. Then I thought, 'wait, if they have this feature, why would calls to their functions with CF format fail ?'. If I swap units of all test_core.py tests for mm d-1 (CF format), everything runs fine. So I am wondering what is the code that caused this issue to be filed. FYI @brews.

Thanks !

[dgergel]

@emileten it was that recent xclim updates included an update in the QDM and QPLAD base class that now checks units to make sure that units for ref and hist in the training step are the same. This step uses pint for the check and so if the units for ref or hist aren't in pint it will err.

My guess is that we may not have checked what you tried, but @brews has been following this more closely than I have.

[emileten]

Thanks @dgergel ! @brews do you have perhaps an idea of which xclim function exactly was triggering this error ? If not it's fine, we can keep my changes and in the worse case we'll be doing a conversion xclim does under the hood anyways.

[dgergel]

Oh sorry, that actually is a question for me. @emileten it was the training step in QDM and AIQPD. Anyway I think it's fine to keep as is.

Looks like this just needs a black for code cleanup to pass tests.

[emileten]

Sure @dgergel, will do that in a bit.

[brews]

Yeah. I saw the xclim changelog on this but I have not seen this fail "in the wild" with precip because of pint and xclim.

[brews]

I think we can do away with all these log messages in dodola.services with this current setup but this merged before I could get a re-review in. Suggestion is now in #148.