-
Notifications
You must be signed in to change notification settings - Fork 0
Conversation
4b9e84b
to
ce24d26
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small issue: I'm getting 2006-2007
and 2099-2100
values from http://localhost:8080/api/climate-data/31/RCP85/indicator/frost_days?time_aggregation=offset_yearly (i.e. it's not excluding spans that are missing one side of the data)
Other than that, just a few small comments below.
|
||
@classmethod | ||
def make_ranges(cls, label): | ||
""" Takes the values of get_intervals and wraps them in CaseRange objects |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stale comment. This one doesn't use get_intervals
.
""" QuerysetGenerator based on a list of period lengths | ||
|
||
Assumes that the periods are consecutive, and that each period takes place immediately | ||
following the previous period. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth adding an __init__
with an assert that the intervals add up to a year? It would just be protecting against errors on our part, but it seems like the sort of error that could crop up and it would be nice for it to present in an obvious way if it does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good thought, I think I'll try to do that in a unit test so we don't have to wait to hit a runtime error to detect a class definition problem.
|
||
|
||
class CustomQuerysetGenerator(QuerysetGenerator): | ||
custom_spans = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a docstring note here that "Custom" means "custom within a year".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2006 and 2100 should be valid dates, they're included in the GDDP data file list for RCP45 and RCP85. Is the data for those intervals coming back weird? If it's getting cut off an indicator like heating_degree_days
will be very low because it only has data for half the winter.
""" QuerysetGenerator based on a list of period lengths | ||
|
||
Assumes that the periods are consecutive, and that each period takes place immediately | ||
following the previous period. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good thought, I think I'll try to do that in a unit test so we don't have to wait to hit a runtime error to detect a class definition problem.
ddf3e8d
to
1cea353
Compare
1cea353
to
dc44877
Compare
The edge year problem should be fixed, turns out some scenarios have data to 2100 and others only to 2099, so we had to add some logic to detect what the year boundaries are for a given data source and filter for the days we can use within that. A bit complex, but thankfully not as bad as feared. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I've somehow destroyed or lost track of my database container, so re-testing that last little change would be a major production. But it looks good. 👍
Overview
Introduces the ability to perform time aggregations across year boundaries, allowing winter measurements to be handled as a single segment using the timespan value
offset_yearly
.Prerequisite for #169
Demo
Notes
Testing Instructions
min_low_temperature
side by side, one fortime_aggregation=offset_yearly
and the other fortime_aggregation=yearly
yearly
instance and compare their min value to the min inoffset_yearly
for winters ending or starting with that yearChecklist
Connects #198