-
Notifications
You must be signed in to change notification settings - Fork 0
Quarterly and User Defined Aggregations #197
Conversation
I had to bump my factory-boy version to 2.8.1 to run tests, see: FactoryBoy/factory_boy#334 Do you? If not, I'll make a separate PR for it. |
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.
Looks sensible to me and the response time is on par with other requests. I appreciated your use of comments. Consider renaming 'interval' variable. Definitely update swagger documentation.
"'stdev' is an alias to 'stddev'. Defaults to 'min,max,avg'.") | ||
|
||
TIME_AGGREGATION_PARAM_DOCSTRING = ("Time granularity to group data by for result structure. Valid " | ||
"aggregations depend on indicator. Can be 'yearly', 'monthly', " |
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.
also, 'quarterly'
@@ -265,6 +265,15 @@ def climate_indicator(request, *args, **kwargs): | |||
required: false | |||
type: string | |||
paramType: query | |||
- name: intervals | |||
description: A list of comma separated month-day pairs defining the time intervals to |
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.
Add something at the start like:
"Used only with 'custom' time aggregation"
@@ -265,6 +265,15 @@ def climate_indicator(request, *args, **kwargs): | |||
required: false |
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.
Update the swagger instructions for time_agg like you did indicators/params.py otherwise it is unclear how to use time_agg vs/and/or interval
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.
idea: 'custom_time_agg', while verbose, would be a more intuitive title instead of 'intervals'
" to aggregate within. Data points will only be assigned to one " | ||
"aggregation, and for overlapping intervals the interval defined first" | ||
" will take precedence. Dates are formmatted MM-DD and pairs are " | ||
"formatted 'start:end'. Examples: '3-1:5-31', '1-1:6-31,7-1:12-31'") |
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.
add some language in here about being for "custom" time_agg
also, 6-31 is not a real date heh
'then': Value(case.index) | ||
}) for case in config['ranges']] | ||
year_whens.append(When(data_source__year__in=config['years'], then=Case(*case_whens))) | ||
# raise Exception( Case(*year_whens, output_field=IntegerField())) |
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.
Whoops, gotta take this out
I didn't run into a problem with Factory Boy myself, did you have to create a new VM for this PR? It may be an issue for VMs provisioned after the change on December 16th. Definitely something to fix, though |
e7de2fb
to
5858cb9
Compare
5858cb9
to
3b1ccbb
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.
Seems good to me
Quarterly and User Defined Aggregations
Overview
With the refactoring done to the indicator code recently, we've introduced the ability to define additional time aggregation formats without having to redefine indicators.
This takes advantage of that flexibility to define two new aggregation formats:
All indicators that previously supported yearly and monthly aggregation now should support these indicators. The previously yearly only indicators remain yearly only.
Testing Instructions
time_aggregation=quarterly
time_aggregation=custom
and theintervals
parameter set to variations of the formatMM-DD:MM-DD
, multiple pairs joined by a,
Closes #137