Degree Days Indicators #154
Conversation
rmartz
force-pushed
the
feature/degree_day_indicator
branch
from
0ee7e6b to
829d22f
Compare
| 'F': (-459.67, 1.8), | ||
| 'C': (-273.15, 1), | ||
| 'K': (0, 1) | ||
| } |
There was a problem hiding this comment.
storage_units should go here, not in the mixin. These conversion factors don't make sense without it.
There was a problem hiding this comment.
I disagree, the converter classes are strictly about converting values from one unit system to another - you can convert 260 Kelvin to Fahrenheit perfectly well without knowing what format the climate data is stored as in the database. Indicators need to know storage units because they're actually accessing values from storage, but the converter itself is just a utility they use to accomplish bigger picture goals.
There was a problem hiding this comment.
Yup, you're right. I was losing sight of the fact that OffsetLinearConverter loads the "from" unit as well.
There was a problem hiding this comment.
There is a slight ambiguity about what the numbers mean, but I think that's an fundamental difficulty in this kind of thing... the numbers don't really mean anything, they're just ratios. In this case Kelvin matches the "base unit", but it doesn't have to - we could move the zero offset over by 50, or double the rate, and since conversions are calculated one unit relative the other it'd still work.
I think the zero-offset numbers might be backwards (Right now they measure what 0K is in local units, rather than have a base-units offset of their local 0 from a common value), but since temperature is likely the only case we'd need a unit converter with a zero-offset I suspect 0K is a good enough base point to calculate everything off of.
| } | ||
|
|
||
|
|
||
| class DegreeTemperatureConverter(LinearConverter): |
There was a problem hiding this comment.
"DegreeTemperature" (and "DegreeUnits" below) don't parse for me. I.e. the name reads as a weird way to describe a temperature converter, then when I read the description I say "oh, it means degree-day". So preferred alternatives would be DegreeDayConverter or, if we don't want to say "Day" since that's not of the essence of what it's doing, maybe DegreeDeltaConverter, since the reason it's not affected by baseline is that it's describing differences in temperature rather than absolute temperatures.
There was a problem hiding this comment.
There are two hard things in computer science: cache invalidation, naming things, and off-by-one errors.
I've been unable to find much in the way of representing non-offset temperature readings except that it's rife with ambiguity. If the current name is categorically insufficient then I could get behind TemperatureDeltaConverter to contrast with the TemperatureConverter (The Degree prefix was for contrast, so keeping it would be redundant with the Delta suffix).
| return self.conversions[start][end] | ||
| return self.converter_class.get(start, end) | ||
|
|
||
| @property |
There was a problem hiding this comment.
@property and @classmethod are built in, but this needs to be a @classproperty, which can be imported from django.utils.decorators. Otherwise it gets returned rather than run in to_dict() and the view serializer doesn't appreciate being handed a decorated method rather than a list.
There was a problem hiding this comment.
Good call, I wasn't aware that existed!
| # List units as a parameter so it gets updated by the query params if it is overriden. | ||
| # This way we can fall back to the units param if we need to handle bare numbers for basetemp | ||
| parameters = {'basetemp': '65F', | ||
| 'units': 'F'} |
There was a problem hiding this comment.
Including units here seems a little weird within the broader context, but it is, after all, a paremeter, so it might be the broader context that's weird. In any case, we have PR #158 to address such questions.
There was a problem hiding this comment.
The goal here is to allow the fallback when basetemp is given as a pure number... for instance, in ?basetemp=65 we'd interpret it as 65F, and ?basetemp=20&units=C would interpret it as 20C and also present the result in Centigrade as well.
We can go the route that basetemp must always have a unit, but this felt more user friendly.
rmartz
force-pushed
the
feature/degree_day_indicator
branch
from
5495f7e to
b2d8c8d
Compare
rmartz
force-pushed
the
feature/degree_day_indicator
branch
2 times, most recently
from
771f5f2 to
1efcbd4
Compare
This enables us to convert basetemp parameters independantly of the indicator's output converter... we can create a TemperatureDelta converter that we use specifically for converting the parameter value to our storage unit.
rmartz
force-pushed
the
feature/degree_day_indicator
branch
from
1efcbd4 to
675a2cd
Compare
Overview
Adds indicators to measure yearly or monthly degree days. Includes both cooling (For days that exceed a baseline) and heating (For days below the baseline) degree days.
Baseline temperature can be set via the
basetempparameter, default to 65F. It can either be specified as a base number (Such as 65), in which case it defaults to being interpreted as the output units (For instance,monthly_heating_degree_days/?units=C&basetemp=20would be interpreted as 20C), or with the unit suffix included (Such as 70F or 290K), likemonthly_heating_degree_days/?basetemp=290K.Notes
Refactors unit conversion to support forward and backward conversion without a full mapping of source to destination conversions. Also adds a new form of "temperature" unit conversion, degree temperature, which ignores the different 0-points for different units. For example, 0C is 32F, but 1 °C is 1.8 °F.
Testing Instructions
Poll the four new indicator endpoints:
Also try with various permutations of
basetempandunitsto verify the unit conversion works.Closes #133