Indicator Parameters #158

CloudNiner · 2016-12-09T15:33:54Z

Overview

Breaks out the definition, documentation and validation of Indicator parameters to a separate class. Like other parts of Indicator, the params_class is provided to each indicator and used to validate them. Any class which needs a different set of IndicatorParams is free to override with its own definition.

See params.py for more documentation about IndicatorParams specifics

Demo

None. The application should continue to function as before.

Notes

The 'percentile' parameter for the extreme events indicators is no longer optional, now that the API throws clear errors if its not provided.

I'm sure there are other things I didn't consider. Happy to consider alternative approaches.

Testing Instructions

All tests should still pass
Use some of the indicator endpoints, passing parameters. They should all continue to function as before.

Closes #148

KlaasH

This looks good. Here are a few notes from looking at the code. I'm having VM issues again, so I'll try it out once I get them resolved.

KlaasH · 2016-12-12T05:05:13Z

django/climate_change_api/indicators/params.py

+        for param in self.PARAMS:
+            value = self._params.get(param, None)
+            validation_fn = getattr(self, 'validate_' + param)
+            clean_value = validation_fn(value)


Might be friendlier to add errors to a list and raise at the end so that we don't keep some errors secret until they've cleared the previous one. On the other hand sometimes all the errors is more than you need, but if we don't expect too many interdependent errors then all at once is probably better.

They way you suggest is generally better I think, as it presents all errors to the user at once. It's also the way DRF does it with their validators, so there would be some consistency there.

If we stick with the bulk of what's in this PR already, I'll switch over.

KlaasH · 2016-12-12T05:05:26Z

django/climate_change_api/indicators/params.py

+    def _check_is_string(self, param, value):
+        """ Private helper method to raise ValidationError if value is not a string """
+        if not isinstance(value, basestring):
+            raise ValidationError('{} must be a string comma separated list'.format(param))


Confusing error message. It can just be a single value, right?

Yeah, I'll clean this up a bit.

KlaasH · 2016-12-12T05:06:55Z

django/climate_change_api/indicators/params.py

+class PercentileIndicatorParams(IndicatorParams):
+    """ Extend IndicatorParams with a non-optional 'percentile' parameter """
+
+    PARAMS = IndicatorParams.PARAMS + ('percentile',)


Though it's simple, a method would probably be better. Could be a class method. If we ever end up with a multi-level class hierarchy, having it be

@classmethod def params(cls): return super(ThisClassName, cls).params + ('percentile',)

would make that smoother (because no parent class name hard-coded outside class declaration).

rmartz

Haven't tested this yet, but left a couple thoughts on different options to consider. If we're good with this way then I'll boot up my CC API box and give it a good run through.

rmartz · 2016-12-12T16:07:20Z

django/climate_change_api/indicators/params.py

+    def validate_years(self, value):
+        return self._check_is_string('years', value) if value is not None else None
+
+    def validate_agg(self, value):


These feel kind of boilerplate-y... since we're using self inspection to detect these functions, we might be able to refactor the parameters themselves into independent classes that handle assignment, validation and default values invisibly internally (Like how fields in a Django model work).

rmartz · 2016-12-12T16:13:09Z

django/climate_change_api/indicators/params.py

+        except ValueError:
+            raise ValidationError('percentile param must be an integer')
+        if int_value < 1 or int_value > 99:
+            raise ValidationError('percentile param must be within the range (0-100)')


I think we can be inclusive in the range, for instance a 100th percentile limit might mean filter for values that are greater than any value ever seen previously, which could be a sensible thing to ask for in certain situations.

Agree, makes sense. Same for the 0th percentile.

rmartz · 2016-12-12T16:19:48Z

django/climate_change_api/indicators/params.py

@@ -0,0 +1,95 @@
+
+class ValidationError(Exception):


Django already provides its own ValidationError exception, might be better to rename this to avoid confusion or switch to using theirs.

Left this separate so we could specifically distinguish a params.ValidationError. Maybe a different name is appropriate?

rmartz · 2016-12-12T16:28:01Z

django/climate_change_api/indicators/params.py

+        return value
+
+
+class PercentileIndicatorParams(IndicatorParams):


I'm kind of on the fence about having too many one-stop-shop classes that define specific combinations of features internally as a black box. Object inheritance is pretty handy, but for things like this I tend to feel that giving the end user the ability to choose which tools they want is more friendly than giving them a pre-defined box that has the tools we think they'll want baked in.

The alternative would be to build parameter detection and handling in an abstract indicator (sub?)class and requiring the indicators define which additionally parameters they want individually. It's a tradeoff between flexibility and risking repeated configuration, but in this case I think parameters have the chance to be sufficiently specific to their individual uses that the extra flexibility could be worthwhile.

Yeah, I'm not super sold on this strategy here, although when I first looked at doing what you describe in your comment above (param classes like the Django field classes), I ran into some trouble with the specifics that would potentially make them unwieldy.

I'll play around with this a bit more this am to see if I can come up with something better.

CloudNiner · 2016-12-15T16:08:12Z

Updated with a new commit that does some refactoring which should better align with the suggestions made. Some additional context from the new commit's log:

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.

There's still the weirdness that some IndicatorParam instances may need indicator specific context. Worked around this for now by adding the Indicator class method that essentially acts as an IndicatorParams factory, and could be overridden by subclasses. IndicatorParams also retains the non-optional init arguments. All ears on a better way to get around this.

Side effect of this refactor is that IndicatorParam instances can be self-documenting, so I added returning the IndicatorParam help text to the API response on 400 errors. Seems good.

Note that I forced pushed to keep the refactor to a single commit.

KlaasH

The new setup looks good. A hierarchy of IndicatorParams classes doesn't seem so onerous when there's zero boilerplate required in the subclasses.

KlaasH · 2016-12-16T16:58:12Z

django/climate_change_api/indicators/params.py

+
+        """
+        value = value if value is not None else self.default
+        if value is None and self.default is None and self.required:


Checking self.default is None is redundant given the previous line, right?

KlaasH · 2016-12-16T18:14:46Z

django/climate_change_api/indicators/abstract_indicators.py

+        Should not validate the IndicatorParams
+
+        """
+        return cls.params_class(cls.default_units, cls.available_units)


What about passing cls and reading the properties off of it in IndicatorParams.__init__? It seems squirrelly, but this mandatory-positional-arguments thing is not pretty either. I feel like passing the class doesn't change the essentials of what's happening but removes one of the moving parts and would make adding further context-sensitive parameters easier. I also am not seeing a way to do this statically,

@rmartz offered the same suggestion. The problem is that then two things happen. One, it makes the relatively simple thing you're trying to decouple still depend on the complex thing, which makes test setup and instantiation far more complicated, and makes the simple thing (IndicatorParams) have a lot of context about things it doesn't care about. Two, it's very dangerously close to a circular dependency because the Indicator creates an instance of IndicatorParams, and at the same time IndicatorParams requires Indicator to initialize.

The design as it stands now is very very close to the Django Model+ModelField/DRF Serializer+SerializerField designs which are all doing the same core things. In the Serializer design, the solution is for the parent class (IndicatorParams) in our case, to take an open-ended 'context' dict. I could explore that, rather than the required positional parameters route.

I left it as two required positional parameters for now because those two params are required for proper functioning of the IndicatorParams so it makes sense to not hide them in a context dict, when in reality you'd always have to provide them. Maybe the solution here is to add context=None to the function signature and docstring now, even though its not used, to guide future changes?

Would it make sense to look at the units from the other direction? Perhaps rather than have the units parameter read state from the indicator about what units it should allow, we create a small set of parallel fixed-scope parameters for each type... like TemperatureUnitsParameter, PrecipitationUnitsParameter, and maybe others as needed, and the indicator use those to define what its units are and what converter it should create?

Yup, makes sense. context sounds good as a way to make it more flexible, but maybe it's better to stop being overprotective of future-us and let them decide whether to add that when it's needed or do something else.

@rmartz potentially. It would require more subclassing of the IndicatorParams by default than there is now, as I think there would need to be an IndicatorParams subclass for each unit group. I'm inclined to hold off for now and revisit later if it becomes more of an issue.

CloudNiner · 2016-12-19T19:15:25Z

Updated with a rebase against develop, which brings in the DegreeDay indicators.

At this point will probably wait to merge this until #162 is merged as well.

rmartz

Not sure if it's too much scope creep for this PR, but do we want to add defined parameters to the indicator serialization output? It would be helpful for the lab to know which indicators support which parameters, but I can totally see that being something we save for another task later on.

rmartz · 2016-12-21T15:24:46Z

django/climate_change_api/indicators/params.py

+
+BASETEMP_PARAM_DOCSTRING = "The base temperature used to calculate the daily difference for degree days summations. See 'basetemp_units' for a discussion of the units this value uses."
+
+BASETEMP_UNITS_PARAM_DOCSTRING = "Units for the value of the 'basetemp' parameter. If not provided, the API assumes the basetemp units match whatever is provided to the 'units' parameter."


What is the advantage for having these set out at the top of the file?

None in particular, I could define them on whichever IndicatorParams class uses them instead. Didn't want them to be inline strings in the description field (where they're used) because it looks odd and was super unwieldy.

rmartz · 2016-12-21T15:26:53Z

django/climate_change_api/indicators/params.py

+    """ Extend IndicatorParams with a non-optional 'percentile' parameter """
+    percentile = IndicatorParam('percentile',
+                                description=PERCENTILE_PARAM_DOCSTRING,
+                                required=True,


Do we not want this to be optional? It used to be hardcoded to 99th percentile before #146, makes sense to me to let it default to that if not set.

Whoops, I don't think that was entirely intentional. I can swap these indicators back to providing a default for this parameter.

It also looks like I made the DegreeDays basetemp param required as well. That should definitely default to 65F, so set the default there to basetemp=65&basetemp_units=F

CloudNiner · 2016-12-21T15:59:40Z

re: Adding the parameters to the Indicator serialization...I will definitely add that. It should be trivial to do since the parameter serialization methods already exist.

CloudNiner · 2016-12-22T15:03:08Z

Hey, its me again. Updated once more with a rebase against develop to include the time aggregation parameter.

Also updated:

IndicatorParams available to a particular Indicator are returned at /api/indicator to improve self-documentation
Updated PercentileIndicatorParams and DegreeDaysIndicatorParams to have defaults for their extra parameters, and adjusted tests accordingly.

Tests should continue to pass, this could use one more look before merging. The commit history is kind of a mess due to the many rebases, so I'll probably rebase this down to one commit with a detailed log message before merging.

rmartz

Looks great! A couple minor comments, only thing that really stands out to me is telling Swagger about basetemp_units, if we get that added then this would be perfect 👌

rmartz · 2016-12-22T17:21:02Z

django/climate_change_api/indicators/params.py

+
+BASETEMP_PARAM_DOCSTRING = "The base temperature used to calculate the daily difference for degree days summations. Defaults to 65. See the 'basetemp_units' for a discussion of the units this value uses."
+
+BASETEMP_UNITS_PARAM_DOCSTRING = "Units for the value of the 'basetemp' parameter. Defaults to 'F'."


Swagger doesn't know about this guy, he still thinks basetemp should include the unit as part of the string

Yeah, this is fallout from now defining our documentation in two places. Hopefully we'll be able to consolidate this once we start building out the documentation stuff I'm working on.

Updated the docstring in views.py regardless.

Yeah, I figured this was a case of things being caught in the middle but thanks for tossing it there for now :)

rmartz · 2016-12-22T17:27:30Z

django/climate_change_api/indicators/params.py

+            param_class.validate(value)
+
+    def to_dict(self):
+        return [c.to_dict() for c in self._get_params_classes()]


If we make this a dictionary we could replace a few higher-level fields like default_units and let them be accessed by [..].parameters.units.default, for example.

Hmm, potentially. Not sure if that would be more trouble than its worth.

Opened #173 to address this down the road.

rmartz · 2016-12-22T17:28:34Z

django/climate_change_api/indicators/params.py

+
+
+class Percentile5IndicatorParams(IndicatorParams):
+    """ Extend IndicatorParams with a non-optional 'percentile' parameter """


Still says non-optional here

rmartz · 2016-12-22T17:28:41Z

django/climate_change_api/indicators/params.py

+
+
+class Percentile95IndicatorParams(IndicatorParams):
+    """ Extend IndicatorParams with a non-optional 'percentile' parameter """


Ditto to non-optional

rmartz · 2016-12-22T17:29:32Z

django/climate_change_api/indicators/params.py

+    percentile = IndicatorParam('percentile',
+                                description=PERCENTILE_5_PARAM_DOCSTRING,
+                                required=False,
+                                default=95,


PERCENTILE_5_PARAM_DOCSTRING says "Defaults to 5"

rmartz · 2016-12-22T17:32:00Z

django/climate_change_api/indicators/params.py

+    if the IndicatorParam in question has no run-time dependencies.
+
+    """
+    models = IndicatorParam('models',


Would it make sense to give this and the years parameter default values? The description says "If not provided, defaults to all models.", but the JSON serialization says default=null which feels a little confusing. Maybe have the serializer omit the default parameter if it's unset?

I think that works for now, I'll do that.

Added #171 to address this further.

rmartz · 2016-12-22T17:37:54Z

django/climate_change_api/indicators/tests/test_indicators.py

@@ -524,18 +536,19 @@ class MonthlyCoolingDegreeDaysTestCase(IndicatorTests, TestCase):
    indicator_class = indicators.CoolingDegreeDays
    indicator_name = 'cooling_degree_days'
    time_aggregation = 'monthly'
-    parameters = {
+    units = 'C'


Good catch on this 👍

rmartz · 2016-12-22T17:44:43Z

django/climate_change_api/indicators/params.py

+        self.time_aggregation = IndicatorParam('time_aggregation',
+                                               description=TIME_AGGREGATION_PARAM_DOCSTRING,
+                                               required=False,
+                                               default='yearly',


Defaulting to 'yearly' is a problem for the raw data indicators, since they don't support yearly aggregation. There is a argument to just move the daily aggregation into their yearly/monthly counterparts and get rid of them, though, so it may end up not being worth being concerned about here.

Hmm, good point. What should we default to then?

Maybe we should consider removing the daily 'raw data' indicators, since they don't provide additional value over the existing raw data endpoint?

I'm down for getting rid of them. I've played with the idea of changing AverageHighTemperature, MaxHighTemperature, etc, to all be HighTemperature, LowTemperature, Precipitation, and the average, max, min, etc, would all be defined by parameter. That would cut down on the number of indicators we need to maintain even further and would give a good use for the daily aggregation if we want to keep it.

Edit We could plan to do that as part of #164 if wanted

Ok. Sounds good. Opened #172 to address removing the daily indicators.

KlaasH

Yup, this is looking very nice.

A few comments and questions, some overlapping with what Reed just said. The one bug I'm seeing is that providing an agg parameter doesn't work (it starts returning empty objects for all data points).

KlaasH · 2016-12-22T16:53:10Z

django/climate_change_api/indicators/params.py

+
+TIME_AGGREGATION_PARAM_DOCSTRING = "Time granularity to group data by for result structure. Valid aggregations depend on indicator. Can be 'yearly', 'monthly' or 'daily'. Defaults to 'yearly'."
+
+UNITS_PARAM_DOCSTRING = "Units in which to return the data. Defaults to Imperial units (Fahrenheit for temperature indicators and inches per day for precipitation)."


Now just "inches" for precip. Or inches per aggregation period, but it's probably clearer not to write that...

KlaasH · 2016-12-22T17:16:59Z

django/climate_change_api/indicators/indicators.py



 class ExtremeHeatEvents(CountUnitsMixin, CountIndicator):
    label = 'Extreme Heat Events'
    description = ('Total number of times per period daily maximum temperature exceeds the '
-                   'specified (Default 99th) percentile of observations from 1960 to 1995')
+                   'specified percentile of observations from 1960 to 1995')
+    params_class = Percentile95IndicatorParams


Was there a discussion around changing the definition of "extreme" from 99th to 95th? Is that more standard? Intuitively, conditions expected on roughly 18 days per year don't seem to rise to the level of "extreme".
For one reference point, Philly's numbers (in F and inches/day):
tasmax 95th: 89.1, tasmax 99th: 93.7
pr 95th: 0.72, pr 99th: 1.36
tasmin 5th: 18.3, tasmin 1st: 9.3

Whoops. No. Lets just leave it at 1/99 for now. Will update.

KlaasH · 2016-12-22T17:44:11Z

django/climate_change_api/indicators/params.py

+    if the IndicatorParam in question has no run-time dependencies.
+
+    """
+    models = IndicatorParam('models',


These base params ('models', 'years', 'agg') do have default values (all, all, 'min, max, avg' respectively), and they say so in the documentation in the view and in their "description" strings, but now that the parameters are generating their own documentation and there's a default property in the objects they return, that seems like the most natural place for people to look, and it's currently not reflecting the actual defaults.

Not sure how much bother would be needed to make them show up and how much we're willing to accept, but it would be nice.

It would be a bit more effort to have years/models handle the special case of an 'all' option. I'll probably leave this as is for now, using Reed's suggestion that we don't display default if its not set. I attempted to set the default for agg to be 'min,max,avg' and things broke. I expect that is related to the issue you noted below.

Can add issue to better handle the years/models param. Wasn't a well exposed issue until now.

Added #171 to address this further.

KlaasH · 2016-12-22T17:49:00Z

django/climate_change_api/indicators/params.py

+                           description=YEARS_PARAM_DOCSTRING,
+                           required=False,
+                           validators=None)
+    agg = IndicatorParam('agg',


Actually, when I try to override agg I get a whole lot of nothing. years and models seem to work fine.

Ok, I'll take a look. I'd expect a test to be failing here, but that doesn't appear to be the case.

Fixed in 439ca70

Added #174 to address the missing test for this bug.

#158 (comment) TODO: Fix agg param so we can give it a static default of 'min,max,avg' to match the docstring.

rmartz

👍 Looks great!

CloudNiner · 2016-12-22T19:37:11Z

Think that should do it for the recent round of comments. Opened issues for related items that don't fit as well in the scope of this (already large) PR.

Overview: Breaks out the definition, documentation and validation of Indicator parameters to a separate class. Like other parts of Indicator, the params_class is provided to each indicator and used to validate them. Any class which needs a different set of IndicatorParams is free to override with its own definition. IndicatorParams contains class or instance defined IndicatorParam classes, which encapsulate definition and validation logic for each parameter. IndicatorParam is self-contained and self-documenting and provides extensible validation via an API similar to the one used in DRF via the serializer classes. Because IndicatorParams are self-documenting, their definition is returned as part of the API response at `/api/indicator/` See params.py for more documentation about IndicatorParams specifics IndicatorParams notes: Updates the agg param to have a self-documented default. The agg param has no e2e test, opened #174 to address this separately. PercentileIndicatorParams notes: This extra parameter remains unchanged. DegreeDayIndicatorParams notes: Handles the DegreeDay indicators via extra 'basetemp' and 'basetemp_units' parameters. Replaces the singular param `basetemp=<value>[K|C|F]` with the two parameters above so that each parameter is only responsible for one thing, which hopefully reduces confusion for users. Fixed a bug in the MonthlyCoolingDegreeDaysIndicatorTestCase where the test case was not performing the correct conversion from basetemp_units.

Indicator Parameters

CloudNiner assigned rmartz and KlaasH Dec 9, 2016

CloudNiner force-pushed the feature/awf/indicator-params branch from b292f28 to cd28b65 Compare December 9, 2016 15:34

KlaasH reviewed Dec 12, 2016

View reviewed changes

rmartz reviewed Dec 12, 2016

View reviewed changes

KlaasH mentioned this pull request Dec 14, 2016

Degree Days Indicators #154

Merged

CloudNiner force-pushed the feature/awf/indicator-params branch from df44184 to 44b97c6 Compare December 15, 2016 16:01

KlaasH reviewed Dec 16, 2016

View reviewed changes

CloudNiner force-pushed the feature/awf/indicator-params branch from 2313222 to 1bec444 Compare December 19, 2016 19:13

rmartz reviewed Dec 21, 2016

View reviewed changes

Fixup manage.py commands in README

7714c5f

CloudNiner force-pushed the feature/awf/indicator-params branch from 1bec444 to a5339a3 Compare December 22, 2016 14:16

rmartz suggested changes Dec 22, 2016

View reviewed changes

KlaasH suggested changes Dec 22, 2016

View reviewed changes

CloudNiner changed the title ~~Feature/awf/indicator params~~ Indicator Parameters Dec 22, 2016

CloudNiner pushed a commit that referenced this pull request Dec 22, 2016

Only show default param if not null

18604c5

#158 (comment) TODO: Fix agg param so we can give it a static default of 'min,max,avg' to match the docstring.

CloudNiner mentioned this pull request Dec 22, 2016

Refactor IndicatorParams to handle other properties of Indicator #173

Open

rmartz approved these changes Dec 22, 2016

View reviewed changes

CloudNiner mentioned this pull request Dec 22, 2016

Add Indicator tests to validate 'agg' parameter #174

Closed

KlaasH approved these changes Dec 23, 2016

View reviewed changes

CloudNiner force-pushed the feature/awf/indicator-params branch from 439ca70 to f5da68a Compare December 23, 2016 13:58

CloudNiner force-pushed the feature/awf/indicator-params branch from f5da68a to 39931f2 Compare December 23, 2016 14:00

CloudNiner merged commit a8c2f86 into develop Dec 23, 2016

CloudNiner deleted the feature/awf/indicator-params branch December 23, 2016 14:02

rmartz mentioned this pull request Dec 29, 2016

Remove raw daily indicators #199

Merged

fungjj92 mentioned this pull request Jan 20, 2017

Add tests between indicator and serializer logic #237

Merged

hectcastro unassigned rmartz Jul 15, 2019

nanotubing unassigned KlaasH Apr 7, 2023

ddohler pushed a commit that referenced this pull request Aug 21, 2023

Merge pull request #158 from azavea/feature/awf/indicator-params

ec8e298

Indicator Parameters

		return value


		class PercentileIndicatorParams(IndicatorParams):


		BASETEMP_PARAM_DOCSTRING = "The base temperature used to calculate the daily difference for degree days summations. See 'basetemp_units' for a discussion of the units this value uses."

		BASETEMP_UNITS_PARAM_DOCSTRING = "Units for the value of the 'basetemp' parameter. If not provided, the API assumes the basetemp units match whatever is provided to the 'units' parameter."



		class Percentile5IndicatorParams(IndicatorParams):
		""" Extend IndicatorParams with a non-optional 'percentile' parameter """



		class Percentile95IndicatorParams(IndicatorParams):
		""" Extend IndicatorParams with a non-optional 'percentile' parameter """


		TIME_AGGREGATION_PARAM_DOCSTRING = "Time granularity to group data by for result structure. Valid aggregations depend on indicator. Can be 'yearly', 'monthly' or 'daily'. Defaults to 'yearly'."

		UNITS_PARAM_DOCSTRING = "Units in which to return the data. Defaults to Imperial units (Fahrenheit for temperature indicators and inches per day for precipitation)."

Indicator Parameters #158

Indicator Parameters #158

Conversation

CloudNiner commented Dec 9, 2016

Overview

Demo

Notes

Testing Instructions

KlaasH left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmartz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CloudNiner commented Dec 15, 2016

KlaasH left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KlaasH Dec 16, 2016 • edited Loading

Choose a reason for hiding this comment

CloudNiner Dec 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CloudNiner commented Dec 19, 2016

rmartz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CloudNiner commented Dec 21, 2016

CloudNiner commented Dec 22, 2016

rmartz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmartz Dec 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KlaasH left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmartz left a comment

Choose a reason for hiding this comment

CloudNiner commented Dec 22, 2016

KlaasH Dec 16, 2016 •

edited

Loading

CloudNiner Dec 19, 2016 •

edited

Loading

rmartz Dec 22, 2016 •

edited

Loading