Degree Day unit tests #160
Conversation
Also fixes a number of undetected issues: - Negative basetemp - Basetemp with invalid suffix unit - Values truncated to integer
| m = re.match(r'^(?P<value>-?\d+(\.\d+)?)(?P<unit>[%s])?$' % ''.join(self.available_units), | ||
| self.parameters['basetemp']) | ||
| if not m: | ||
| raise ValueError('Parameter basetemp must be numeric and end with C, F or K') |
There was a problem hiding this comment.
Should update error string to reference self.available_units
There was a problem hiding this comment.
I'm for that, left it like this to make it read nicer. Which do you think is a better way to go?
- Do a simple join so the list reads kind of funny ("...and end with one of [C, F, K]")
- Write a slightly more complex function to do two joins so we use proper English conjunctions ("...and end with C, F or K")?
There was a problem hiding this comment.
In other places, and particularly in the params PR, I've kept it simple and just let python format complex objects like lists/sets via format() or str(). Not that much worse IMO and a lot less effort. And since its an API it would likely be mostly tech savvy people reading these messages.
| def __init__(self, *args, **kwargs): | ||
| super(BasetempIndicatorMixin, self).__init__(*args, **kwargs) | ||
|
|
||
| m = re.match(r'^(?P<value>-?\d+(\.\d+)?)(?P<unit>[%s])?$' % ''.join(self.available_units), |
There was a problem hiding this comment.
Now that you're using self.available_units here, and that can be things other than temp units, it might be good to also throw an exception if self.variables contains anything other than tasmin or tasmax.
There was a problem hiding this comment.
That's a good point, I may need to change this to be TemperatureConverter.units specifically, since there could possibly be a case where a non-temperature indicator has a temperature input (Say, "Yearly Snowfall" that gives total precipitation on days with tasmin or tasmax below a given threshold).
There was a problem hiding this comment.
That seems even better.
The BasetempIndicatorMixin previously assumed that self.available_units would match the TemperatureConverter's units, but it isn't necessarily true that an indicator that accepts a basetemp parameter is a temperature parameter... for instance, a precipitation indicator may use temperature to filter which days to inspect.
Overview
The Degree Day PR (#154) neglected to include unit tests for the new indicators. This corrects this and fixes a number of issues encountered:
Testing Instructions