Refactor IndicatorParams to use IndicatorParam instances + validators

Replaces validate_paramname() validation with IndicatorParam instances
that are fully self-contained. IndicatorParam is self-documenting via
its to_dict() method, and provides extensible validation via the same
API as Django and DRF.

While not necessarily simpler, this eases the process for adding
additional params to an indicator. It is still necessary to
subclass IndicatorParams, but all of the logic for new parameters
is fully encapsulated in IndicatorParam.

django/climate_change_api/climate_data/views.py

@@ -2,6 +2,7 @@
 import logging
 
 from django.contrib.gis.geos import Point
+from django.core.exceptions import ValidationError
 from django.core.urlresolvers import reverse
 from django.http import HttpResponseRedirect
 from django.utils.http import urlencode
@@ -20,7 +21,8 @@
                                       ClimateModelSerializer,
                                       ClimateCityScenarioDataSerializer,
                                       ScenarioSerializer)
-from indicators import indicator_factory, list_available_indicators, params
+from indicators import indicator_factory, list_available_indicators
+
 
 logger = logging.getLogger(__name__)
 
@@ -292,16 +294,21 @@ def climate_indicator(request, *args, **kwargs):
         raise ParseError(detail='Must provide a valid indicator')
     try:
         indicator_class = IndicatorClass(city, scenario, parameters=request.query_params)
-    except params.ValidationError as e:
-        raise serializers.ValidationError(str(e))
+    except ValidationError as e:
+        # If indicator class/params fails validation, return error with help text for
+        # as much context as possible.
+        return Response(OrderedDict([
+            ('error', str(e)),
+            ('help', IndicatorClass.init_params_class().to_dict()),
+        ]), status=status.HTTP_400_BAD_REQUEST)
     data = indicator_class.calculate()
 
     return Response(OrderedDict([
         ('city', CitySerializer(city).data),
         ('scenario', scenario.name),
         ('indicator', IndicatorClass.to_dict()),
         ('climate_models', [m.name for m in model_list]),
-        ('units', indicator_class.params.units),
+        ('units', indicator_class.params.units.value),
         ('data', data),
     ]))
 

django/climate_change_api/indicators/abstract_indicators.py

@@ -12,6 +12,7 @@
 from climate_data.filters import ClimateDataFilterSet
 from .params import IndicatorParams
 from .serializers import IndicatorSerializer
+from .validators import ChoicesValidator
 from .unit_converters import DaysUnitsMixin
 
 
@@ -41,14 +42,23 @@ def __init__(self, city, scenario, parameters=None):
         self.city = city
         self.scenario = scenario
 
-        self.params = self.params_class(parameters, self.available_units, self.default_units)
-        self.params.validate()
+        self.params = self.init_params_class()
+        self.params.validate(parameters)
 
         self.queryset = self.get_queryset()
         self.queryset = self.filter_objects()
 
         self.serializer = self.serializer_class()
 
+    @classmethod
+    def init_params_class(cls):
+        """ Return the instantiated IndicatorParams object for this class
+
+        Should not validate the IndicatorParams
+
+        """
+        return cls.params_class(cls.default_units, cls.available_units)
+
     @classmethod
     def name(cls):
         def convert(name):
@@ -80,8 +90,8 @@ def get_queryset(self):
         filter_set = ClimateDataFilterSet()
         queryset = (ClimateData.objects.filter(map_cell=self.city.map_cell)
                     .filter(data_source__scenario=self.scenario))
-        queryset = filter_set.filter_years(queryset, self.params.years)
-        queryset = filter_set.filter_models(queryset, self.params.models)
+        queryset = filter_set.filter_years(queryset, self.params.years.value)
+        queryset = filter_set.filter_models(queryset, self.params.models.value)
         return queryset
 
     def filter_objects(self):
@@ -106,9 +116,9 @@ def convert_units(self, aggregations):
 
         @param aggregations list-of-dicts returned by aggregate method
         @returns Dict in same format as the aggregations parameter, with values converted
-                 to `self.params.units`
+                 to `self.params.units.value`
         """
-        converter = self.getConverter(self.storage_units, self.params.units)
+        converter = self.getConverter(self.storage_units, self.params.units.value)
         for item in aggregations:
             if item['value'] is not None:
                 item['value'] = converter(item['value'])
@@ -143,7 +153,7 @@ def calculate(self):
         aggregations = self.convert_units(aggregations)
         collations = self.collate_results(aggregations)
         return self.serializer.to_representation(collations,
-                                                 aggregations=self.params.agg)
+                                                 aggregations=self.params.agg.value)
 
 
 class YearlyIndicator(Indicator):

django/climate_change_api/indicators/indicators.py

@@ -93,40 +93,40 @@ def aggregate(self):
 class YearlyExtremePrecipitationEvents(CountUnitsMixin, YearlyCountIndicator):
     label = 'Yearly Extreme Precipitation Events'
     description = ('Total number of times per year daily precipitation exceeds the specified '
-                   '(Default 99th) percentile of observations from 1960 to 1995')
+                   'percentile of observations from 1960 to 1995')
     params_class = PercentileIndicatorParams
     variables = ('pr',)
 
     @property
     def conditions(self):
         return {'pr__gt': F('map_cell__baseline__pr'),
-                'map_cell__baseline__percentile': self.params.percentile}
+                'map_cell__baseline__percentile': self.params.percentile.value}
 
 
 class YearlyExtremeHeatEvents(CountUnitsMixin, YearlyCountIndicator):
     label = 'Yearly Extreme Heat Events'
     description = ('Total number of times per year daily maximum temperature exceeds the specified '
-                   '(Default 99th) percentile of observations from 1960 to 1995')
+                   'percentile of observations from 1960 to 1995')
     params_class = PercentileIndicatorParams
     variables = ('tasmax',)
 
     @property
     def conditions(self):
         return {'tasmax__gt': F('map_cell__baseline__tasmax'),
-                'map_cell__baseline__percentile': self.params.percentile}
+                'map_cell__baseline__percentile': self.params.percentile.value}
 
 
 class YearlyExtremeColdEvents(CountUnitsMixin, YearlyCountIndicator):
     label = 'Yearly Extreme Cold Events'
     description = ('Total number of times per year daily minimum temperature is below the specified '
-                   '(Default 1st) percentile of observations from 1960 to 1995')
+                   'percentile of observations from 1960 to 1995')
     params_class = PercentileIndicatorParams
     variables = ('tasmin',)
 
     @property
     def conditions(self):
         return {'tasmin__lt': F('map_cell__baseline__tasmin'),
-                'map_cell__baseline__percentile': self.params.percentile}
+                'map_cell__baseline__percentile': self.params.percentile.value}
 
 
 class HeatWaveDurationIndex(YearlyMaxConsecutiveDaysIndicator):
@@ -195,40 +195,40 @@ class MonthlyFrostDays(DaysUnitsMixin, MonthlyCountIndicator):
 class MonthlyExtremePrecipitationEvents(CountUnitsMixin, MonthlyCountIndicator):
     label = 'Monthly Extreme Precipitation Events'
     description = ('Total number of times per month daily precipitation exceeds the specified '
-                   '(Default 99th) percentile of observations from 1960 to 1995')
+                   'percentile of observations from 1960 to 1995')
     params_class = PercentileIndicatorParams
     variables = ('pr',)
 
     @property
     def conditions(self):
         return {'pr__gt': F('map_cell__baseline__pr'),
-                'map_cell__baseline__percentile': self.params.percentile}
+                'map_cell__baseline__percentile': self.params.percentile.value}
 
 
 class MonthlyExtremeHeatEvents(CountUnitsMixin, MonthlyCountIndicator):
     label = 'Monthly Extreme Heat Events'
     description = ('Total number of times per month daily maximum temperature exceeds the specified '
-                   '(Default 99th) percentile of observations from 1960 to 1995')
+                   'percentile of observations from 1960 to 1995')
     params_class = PercentileIndicatorParams
     variables = ('tasmax',)
 
     @property
     def conditions(self):
         return {'tasmax__gt': F('map_cell__baseline__tasmax'),
-                'map_cell__baseline__percentile': self.params.percentile}
+                'map_cell__baseline__percentile': self.params.percentile.value}
 
 
 class MonthlyExtremeColdEvents(CountUnitsMixin, MonthlyCountIndicator):
     label = 'Monthly Extreme Cold Events'
     description = ('Total number of times per month daily minimum temperature is below the specified '
-                   '(Default 1st) percentile of observations from 1960 to 1995')
+                   'percentile of observations from 1960 to 1995')
     params_class = PercentileIndicatorParams
     variables = ('tasmin',)
 
     @property
     def conditions(self):
         return {'tasmin__lt': F('map_cell__baseline__tasmin'),
-                'map_cell__baseline__percentile': self.params.percentile}
+                'map_cell__baseline__percentile': self.params.percentile.value}