Move variable mapping out of mpas_xarray into new generalized_reader module #104

xylar · 2017-02-08T17:37:12Z

This merge moves the capability to map variable names (used in MPAS-Analysis to support multiple MPAS and ACME versions as seamlessly as possible) out of mpas_xarray to a new generalized_reader module.

Both mpas_xarray and generalized_reader have functions, open_multifile_dataset, that can be used in place of existing calls to xarray.open_mfdataset. These functions hide the complications of using a preprocessing function. generalized_reader.open_multifile_dataset adds the capabilities to map variables and to select only 'Time' values between a given start and end date.

These changes have been made to allow MPAS files to be preprocessed in various ways without explicitly having to add complicating functionality to mpas_xarray itself. After these changes, the thought is that mpas_xarray can remain generic to any MPAS data set, whereas generalized_reader can adapt to the specific needs for reading data sets within MPAS-Analysis.

xylar · 2017-02-08T17:39:58Z

@pwolfram and @milenaveneziani, this is the last reorganization PR before I do a pull request to generalize the calendar. This builds on #102, so should not be merged until that PR has been fulfilled.

This is a relatively simple reorganization, so I didn't feel like it needed a design document. Please let me know if you disagree and would prefer to have a design document.

xylar · 2017-02-08T17:42:20Z

Oops, I need to add CI for this and fix the mpas_xarray CI. Update coming soon...

xylar · 2017-02-08T18:02:46Z

Okay, I added CI.

pwolfram · 2017-02-08T21:30:56Z

@xylar, once we fulfill #86 do you mind doing the rebase so that this is easier to review?

xylar · 2017-02-08T22:08:34Z

Yes, I'll definitely do that.

xylar · 2017-02-10T16:39:39Z

@pwolfram, probably hold off on reviewing this until #102 has been merged. If you like, you can take a look at the new classes specifically:
https://github.com/MPAS-Dev/MPAS-Analysis/pull/104/files#diff-bd27288e6337dce2ebec4ac95896b8eb
https://github.com/MPAS-Dev/MPAS-Analysis/pull/104/files#diff-27af017bf6b93f74592ce0c6fca4cc1b

One question I have for you is how you feel about my file organization:

mpas_analysis/shared/MpasXarray/MpasXarray.py
mpas_analysis/shared/MpasAnalysisXarray/MpasAnalysisXarray.py

This organization leads to clunky imports like:

from ..shared.MpasAnalysisXarray.MpasAnalysisXarray import openMultifileDataSet

Also, would you prefer the files and directories not be named after the classes they contain, given that there are also several utility functions in these files? Would you prefer I separate the utility functions into their own files (e.g. utility.py) like we have in other subfolders?

I didn't want to bother with a design doc for this PR but this particular bit of decision making probably would have warranted a design doc. Let me know if you feel strongly that I should make one.

milenaveneziani · 2017-02-10T17:05:44Z

@xylar: this needs a rebase and an opinion from @pwolfram about the question above, right?

xylar · 2017-02-10T17:08:54Z

Okay, this one is rebased and ready to review.

milenaveneziani · 2017-02-10T17:12:39Z

mpas_analysis/ocean/ohc_timeseries.py

-
-        dsFirstYear = remove_repeated_time_index(dsFirstYear)
+        dsFirstYear = openMultifileDataSet(filesFirstYear,
+                                           calendar,


minor comment, but shouldn't the two lines above have inFiles=filesFirstYear and calendar=calendar, for style consistency?

Yes, I agree that would be an improvement.

milenaveneziani · 2017-02-10T17:13:08Z

mpas_analysis/ocean/ohc_timeseries.py

-            preprocess=lambda x: preprocess_mpas(x, yearoffset=yearOffset))
-        dsPreprocessed = remove_repeated_time_index(dsPreprocessed)
+        dsPreprocessed = openMultifileDataSet(inFilesPreprocessed,
+                                              calendar,


same minor comment as above

milenaveneziani · 2017-02-10T17:16:26Z

mpas_analysis/ocean/sst_timeseries.py

-            preprocess=lambda x: preprocess_mpas(x, yearoffset=yearOffset))
-        dsPreprocessed = remove_repeated_time_index(dsPreprocessed)
+        dsPreprocessed = openMultifileDataSet(inFilesPreprocessed,
+                                              calendar,


same comment

milenaveneziani · 2017-02-10T17:18:36Z

mpas_analysis/sea_ice/modelvsobs.py

                               endDate=endDate, calendar=calendar)
-    print 'Reading files {} through {}'.format(infiles[0], infiles[-1])
+    print 'Reading files {} through {}'.format(inFiles[0], inFiles[-1])

    plotsDirectory = buildConfigFullPath(config, 'output', 'plotsSubdirectory')
    obsDirectory = config.get('seaIceObservations', 'baseDirectory')


I know this is not the right place to do it, but I realized recently that we should have the iceCocentration and iceThickness observational files defined in the config file, rather than hard wired here...

Yep, maybe make an issue for this.

milenaveneziani · 2017-02-10T17:20:04Z

mpas_analysis/sea_ice/timeseries.py

-            inFilesPreprocessed,
-            preprocess=lambda x: preprocess_mpas(x, yearoffset=yearOffset))
+        dsPreprocessed = openMultifileDataSet(inFilesPreprocessed,
+                                              calendar,


same comment as before, with a few other instances below.

pwolfram

Big picture concern: use of classes is making internals of mpas_xarray more confusing, but at the benefit of providing a single function a user needs to understand in order to load in data. There is a trade-off here and in some ways this assumes that our user's won't be developers. This is potentially dangerous given our group's mission and composition.

We definitely need a design document for this code because it is becoming more complex and it will be something everyone will have to live with in the future. I recommend getting feedback from @vanroekel on whether he thinks this all is a step in the right direction because he is less steeped in this project than @xylar, @milenaveneziani, or myself (@pwolfram).

pwolfram · 2017-02-10T17:17:13Z

mpas_analysis/ocean/ohc_timeseries.py

 from ..shared.plot.plotting import timeseries_analysis_plot

 from ..shared.io import NameList, StreamsFile
 from ..shared.io.utility import buildConfigFullPath

+from ..shared.MpasAnalysisXarray.MpasAnalysisXarray import openMultifileDataSet


@xylar, I think this organization is strange. Do we really need a folder with the same class name? Also, typically files with classes are not camel-cased, but would be something like from ...shared.mpas_analysis_xarray import OpenMultifileDataSet where I'm assuming OpenMultifileDataSet is a class. If it is a function it should be lower case with _ characters.

pwolfram · 2017-02-10T17:18:05Z

mpas_analysis/ocean/ocean_modelvsobs.py

+                              variableMap=variableMap,
+                              startDate=startDate,
+                              endDate=endDate,
+                              yearOffset=yearOffset)


Note, this changes public API so we probably want at least a very simple design doc.

pwolfram · 2017-02-10T17:19:38Z

mpas_analysis/sea_ice/timeseries.py

@@ -14,6 +11,9 @@
 from ..shared.timekeeping.utility import stringToDatetime, \
    clampToNumpyDatetime64

+from ..shared.MpasAnalysisXarray.MpasAnalysisXarray import openMultifileDataSet
+from ..shared.MpasXarray.MpasXarray import subsetVariables


You are right @xylar, the organization here is important and need further consideration. I'm thinking we probably do need a design doc

highlighting directory structure

highlighting public API

I don't think this needs to be a "full" design doc, but more of a simple listing of how this all should work.

pwolfram · 2017-02-10T17:20:31Z

mpas_analysis/shared/MpasAnalysisXarray/MpasAnalysisXarray.py

+
+Utility function
+----------------
+openMultifileDataSet : creates an instance of MpasAnalysisXarray, opens a data


Functions typically should not be camel-cased.

milenaveneziani · 2017-02-10T17:26:17Z

I do not have specific opinions about MpasXarray and MpasAnalysisXarray.
About the file organization, I just wonder if we could have both classes in the shared/MpasXarray directory.

pwolfram · 2017-02-10T17:27:27Z

@xylar and @milenaveneziani, I think we want a high-level view of the changes here to directory structure and public API. I'm thinking a minimal quasi-"design doc" may be the best way to do this because we need to make sure these changes are going to be more broadly useful than for just the three of us.

xylar · 2017-02-10T17:50:02Z

The high level changes to the directory structure are quite simple. The following have been added:

mpas_analysis/shared/
             - MpasXarray/
                    __init__.py
                    MpasXarray.py
             -  MpasAnalysisXarray/
                     __init__.py
                    MpasAnalysisXarray.py

And the following has been removed:

mpas_analysis/shared/
             - mpas_xarray/
                    __init__.py
                    mpas_xarray.py

milenaveneziani · 2017-02-10T18:01:10Z

@pwolfram: I am not too concerned about the fact that this PR changes API, since for now we only have 4 developers. But I agree that it would be nice to hear from @vanroekel about these changes.

xylar · 2017-02-11T22:03:12Z

Testing

I tested this with both LANL tests and it's bit-for-bit with #110 (which fixes a bug in develop).

milenaveneziani · 2017-02-11T22:34:55Z

@xylar: I don't have much else to say about this PR or #108. I agree with @vanroekel that the call to open the multi-file dataset is much more intuitive now. Thanks for all the work on this.

xylar · 2017-02-15T22:49:36Z

@pwolfram, once you are happy with the design doc (#108), could you please re-review this PR at your earliest convenience? Much appreciated!

pwolfram

The API for opening files is now much cleaner and simpler, which is a huge plus.

I think this PR will make it easier to load in MPAS data for new users.

A few minor, general notes:

We should try to keep blocks of code as close to their original locations as possible in order to make incremental changes to the code obvious between commits for debugging purposes.
Name changes to use _ characters are general frowned upon in python because strings with _ characters typically designate functions and methods.

pwolfram · 2017-02-16T18:30:12Z

mpas_analysis/ocean/ohc_timeseries.py

+                                             variable_map=variableMap,
+                                             start_date=startDateFirstYear,
+                                             end_date=endDateFirstYear,
+                                             year_offset=yearOffset)


This makes the usage much clearer, especially because additional selections via xarray aren't needed now.

pwolfram · 2017-02-16T18:31:31Z

mpas_analysis/shared/generalized_reader/generalized_reader.py

+    """
+    Overloads the _preprocess function from MpasXarray to include
+    mapping variable names.  See MpasXaray._preprocess for more
+    details.


@xylar, we aren't overloading anymore, right? If so, the docstring needs changed.

Oops, missed that.

pwolfram · 2017-02-16T18:33:09Z

mpas_analysis/shared/mpas_xarray/mpas_xarray.py

    """

    allvars = ds.data_vars.keys()

    # get set of variables to drop (all ds variables not in vlist)
-    dropvars = set(allvars) - set(vlist)
+    dropvars = set(allvars) - set(variable_list)


Functions use underscores, not variables names. This change is confusing and should be rolled-back.

As I said below, I'm really frustrated with this process. I would like some very clear rules on what our naming conventions are. What I feel like I'm getting instead is a steady stream of corrections with no clear rule behind them. What's more I can't find any clear rule in PEP8 either.

I think vlist is a poor variable name. It is not very descriptive. I'm okay with not having an underscore but I think we want descriptive names over short names. I am fine with variableList as an alternative.

All that being said, this is your code so I will defer to your style choices. I just want to understand what the rules are and also to get a chance to say whether I agree with them.

I thought at some point we agreed to use camelcase for variables and underscores for functions/scripts? In any terms, let us just agree on something. My only big preference is for longer rather than shorter/cryptic names, but either camelcase or underscores would work equally well for me.

I guess I didn't understand that. I just talked with @pwolfram and we agreed that CamelCase is the right way to do this for variable names (first letter lowercase) and class names (first letter uppercase). Function and method names as well as directories and file names are all lowercase with underscores.

As a counterexample to this, let me present xarray:

def open_dataset(filename_or_obj, group=None, decode_cf=True, mask_and_scale=True, decode_times=True, concat_characters=True, decode_coords=True, engine=None, chunks=None, lock=None, cache=None, drop_variables=None):

Clearly they don't mind using underscores in variable names.

And I personally do not care either. But I will add this 'rule' in the python standard requirement somewhere, so we can refer to it.

I updated the confluence page with all our requirements:
https://acme-climate.atlassian.net/wiki/display/OCNICE/Standard+for+MPAS+Analysis+scripts

Can we have some type of local document on standards in this repo? It will be easier to refer to if it is local to this repository and not on confluence, although a mirror on confluence may be optimal.

@xylar, you are right that underscored variable names are used in xarray. I would personally say that is suboptimal for us, particularly since we are allowing use of camel case for variable names. I'm happy to bend to the group if you and @milenaveneziani find this too restrictive, however.

pwolfram · 2017-02-16T18:34:24Z

mpas_analysis/shared/mpas_xarray/mpas_xarray.py

    """
    assert datetimes[0].year > 1678, \
-        'ERROR: yearoffset={}'.format(yearoffset) + \
+        'ERROR: year_offset={}'.format(year_offset) + \


Same comment as before. This change is not helpful, but is confusing and against PEP8 and standard conventions.

As I mentioned, this is not covered by PEP8. But I agree that I will change it. Not back to yearoffset but to yearOffset.

pwolfram · 2017-02-16T18:35:38Z

mpas_analysis/shared/mpas_xarray/mpas_xarray.py

        datetimes = [starts[i] + (ends[i] - starts[i])/2
                     for i in range(len(starts))]
        return datetimes

-    time_var = ds[timestr]
+    time_var = ds[time_variable_name]


Use of underscore for variable, again...

Yep, I'll fix it again.

pwolfram · 2017-02-16T18:40:47Z

mpas_analysis/test/test_generalized_reader.py

+                                        end_date='0005-03-01',
+                                        year_offset=1850)
+            self.assertEqual(len(ds.Time), 1)
+


This is really great @xylar -- testing of opening file will hopefully ensure that we have all the functionality we need and if libraries change and this breaks we should know!

pwolfram · 2017-02-16T18:43:23Z

mpas_analysis/shared/generalized_reader/generalized_reader.py

+    02/15/2017
+    """
+
+    def preprocess_fn(ds):


You can use a partial function here, which would be cleaner because you are essentially doing this by hand. See https://docs.python.org/2/library/functools.html#functools.partial

Thanks, that's a great tip!

pwolfram · 2017-02-16T18:44:08Z

mpas_analysis/shared/generalized_reader/generalized_reader.py

+    # and handle it manually (adding a new variable rather than renaming
+    # an existing one)
+    if variable_map is not None and \
+            time_variable_name in variable_map:


I don't know if you need to break this line, it may be clearer (even if the line is long) to keep on one line.

You are right that it doesn't need to be split in this case.

pwolfram · 2017-02-16T18:46:05Z

mpas_analysis/shared/mpas_xarray/mpas_xarray.py

+    return ds  # }}}
+
+
+def preprocess(ds, time_variable_name, variable_list, sel_values,


This code has been largely unchanged, but it is highlighted as though it were new code. In the future, please try to keep edits to code in-place so that we have a clearer idea of how the code evolves from commit to commit, which is obviously very important for debugging.

But, this is probably not a huge deal in this case, but is a huge deal in other contexts.

I am trying to reorganize which functions are public and which are private. This required moving several functions to the end of the file (since my understanding is that private functions should follow public ones whenever possible). GitHub makes arbitrary choices about which functions it follows and which it loses track of in such cases.

In any case, the order of the functions was not arbitrary. It is somewhat related to the history of me making a class and then undoing it per your previous requests.

Ok, sounds good. This is a downside of github and diffs in general-- they really can't sort out moves and edits in general. Seems like there is a better way to do this but no real choices for better diff evaluations...

pwolfram · 2017-02-16T18:47:37Z

mpas_analysis/shared/mpas_xarray/mpas_xarray.py

+    `isel_values`) was provided in `openMultifileDataSet`, this
+    function performs the appropriate slicing on the data set.
+
+    Parameters


Do we know that these current doc strings will be parse-able via sphinx? I don't think we need to verify this now as long as we are willing to make tweaks to the docstrings later when we build out a automatic doc generation capability.

I'm sure they will need tweaks, but I'm doing my best to follow examples I've seen that are parsed with sphinx.

That is exactly what I'd do too. Sounds great.

xylar · 2017-02-16T19:11:57Z

Name changes to use _ characters are general frowned upon in python because strings with _ characters typically designate functions and methods.

@pwolfram, I'm really confused about what you want in terms of CamelCase vs. underscores vs nothing at all. I didn't see anything in the PEP8 prescriptions that agrees with what you just said. I really, really, really hate variable names that are all lowercase with no indication of word separation (more than underscores even). If I had to choose, I would always use CamelCase. You've told me not to except for class names (which I'm apparently not supposed to use anyway because classes are confusing). I'm really frustrated and confused about this!!!

xylar · 2017-02-16T20:11:08Z

@pwolfram, looking again at https://www.python.org/dev/peps/pep-0008/#prescriptive-naming-conventions, I don't see anything about variable names.

pwolfram · 2017-02-16T21:09:37Z

@xylar, to summarize from our phone conversation earlier today:

variable names should be fully lower-case or camel cased with the first letter lower case, e.g., avariablename, aVariableName
function names should be all lower case with underscores, a_function_name
class names should be camel case with a capital first letter, AClassName, or MyClassName
module names should be lower case and may have underscores, a_module_name.py

When in doubt we should try to follow conventions of the pydata stack, e.g., numpy, scipy, pandas, xarray, etc because PEP8 isn't fully prescriptive. In general camel case is probably discouraged in these efforts for variable names but we are allowing it here for consistency with the MPAS convention to meld with existing python standards.

xylar · 2017-02-16T21:25:39Z

@pwolfram, so I've looked at both numpy and xarray, and I see plenty of example where they have function arguments that use lowercase with underscores. See my earlier comment for an example from xarray (xarray.open_dataset). Here's one from numpy.ma.core:

def set_fill_value(a, fill_value):

I don't see much evidence that this convention is as established as you seem to think it is.

pwolfram · 2017-02-16T21:34:14Z

Fair enough @xylar, maybe it is just my own personal quirk then. Vim takes care of this issue anyway by identifying and highlighting functions so this isn't a huge problem, but I thought it could be a helpful convention. I'll go with whatever you think best, but my vote still is a clear distinction between function/method names, variable names, and class names.

xylar · 2017-02-16T21:34:27Z

So let's stick with the convention @pwolfram outlined above. But let's be careful about referring to it as an established standard in python because I don't see much evidence that that's the case. Especially in xarray, it seems not to be the case.

A new utility function, open_multifile_dataset, has been added to make it easier and more straightforward to open multifile datasets without having to understand the preprocessor. Several helper functions have been made non-public because they are unlikely to be used directly outside of mpas_xarray. These include: _assert_valid_datetimes, _assert_valid_selections, _ensure_list, _get_datetimes Unit tests have been updated to use open_multifile_dataset but analysis scripts have not yet been updated because they require an alternative open_multifile_dataset and a more complex preprocess function that can handle variable mapping to support the full MPAS-analysis capabilities.

This module includes: * A utility function open_multifile_dataset, similar to that for mpas_xarray, that opens an xarray data set and does the preprocessing needed for MPAS-Analysis, including mapping variable names and slicing the time axis to the appropriate range. * some non-public helper functions for performing mapping of variable names. Unit testing has also been added for generalized_reader

xylar · 2017-02-16T22:08:08Z

Okay, I really hope I've addressed all your concerns. @pwolfram, could you give it another look when you can?

xylar · 2017-02-16T22:22:37Z

I retested this on my laptop. Seems to work, but I don't have a way of testing sea_ice.modelvsobs still. I'll be away from my computer for the weekend starting tomorrow so won't be able to do any further testing.

@milenaveneziani, would you be willing to try this on all the lanl and edison configs to make sure they still work as expected?

If so, and if @pwolfram doesn't have further changes, you're free to merge. I've already squashed commits.

milenaveneziani · 2017-02-16T22:26:11Z

sure, I will test on edison and wolf and report here.

pwolfram · 2017-02-16T22:54:52Z

I've signed-off.

milenaveneziani · 2017-02-16T22:55:46Z

Great. And my tests just finished as well, successfully: on wolf (GMPAS-240 case) and on edison (beta0 case).

xylar added clean up mpas_xarray labels Feb 8, 2017

xylar assigned pwolfram and milenaveneziani Feb 8, 2017

xylar added the in progress label Feb 8, 2017

xylar force-pushed the variable_mapping_reorg branch from 066d185 to 833171d Compare February 8, 2017 19:53

milenaveneziani mentioned this pull request Feb 8, 2017

Reorganize config.tempalate (formerly config.analysis) #86

Merged

xylar force-pushed the variable_mapping_reorg branch 2 times, most recently from 71f7e57 to 74b7bbf Compare February 10, 2017 16:16

xylar changed the title ~~Reorganize variable name mapping~~ Make MpasXarray and MpasAnalysisXarray classes Feb 10, 2017

xylar force-pushed the variable_mapping_reorg branch from 74b7bbf to 447891a Compare February 10, 2017 17:07

milenaveneziani reviewed Feb 10, 2017

View reviewed changes

xylar mentioned this pull request Feb 10, 2017

Design Document: Generalize Calendar supported by Analysis #105

Merged

milenaveneziani reviewed Feb 10, 2017

View reviewed changes

pwolfram suggested changes Feb 10, 2017

View reviewed changes

xylar force-pushed the variable_mapping_reorg branch from 32e5350 to cb78a8f Compare February 11, 2017 21:55

xylar force-pushed the variable_mapping_reorg branch from cb78a8f to 5e56c59 Compare February 12, 2017 09:10

xylar removed the in progress label Feb 12, 2017

xylar unassigned milenaveneziani Feb 12, 2017

xylar force-pushed the variable_mapping_reorg branch from 5e56c59 to 8e38579 Compare February 15, 2017 22:02

xylar changed the title ~~Make MpasXarray and MpasAnalysisXarray classes~~ Move variable mapping out of mpas_xarray into new generalized_reader module Feb 15, 2017

xylar mentioned this pull request Feb 16, 2017

Design Document: Moving variable mapping out of mpas_xarray #108

Merged

pwolfram suggested changes Feb 16, 2017

View reviewed changes

xylar added 2 commits February 16, 2017 23:03

xylar force-pushed the variable_mapping_reorg branch from 8e38579 to c42fb0e Compare February 16, 2017 22:04

Switch analysis scripts to open_multifile_datset

ce20177

xylar force-pushed the variable_mapping_reorg branch from c42fb0e to ce20177 Compare February 16, 2017 22:18

pwolfram approved these changes Feb 16, 2017

View reviewed changes

milenaveneziani merged commit ce20177 into MPAS-Dev:develop Feb 16, 2017

xylar deleted the variable_mapping_reorg branch February 23, 2017 10:52

		return ds # }}}


		def preprocess(ds, time_variable_name, variable_list, sel_values,

Move variable mapping out of mpas_xarray into new generalized_reader module #104

Move variable mapping out of mpas_xarray into new generalized_reader module #104

Uh oh!

Conversation

xylar commented Feb 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xylar commented Feb 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xylar commented Feb 8, 2017

Uh oh!

xylar commented Feb 8, 2017

Uh oh!

pwolfram commented Feb 8, 2017

Uh oh!

xylar commented Feb 8, 2017

Uh oh!

xylar commented Feb 10, 2017

Uh oh!

milenaveneziani commented Feb 10, 2017

Uh oh!

xylar commented Feb 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pwolfram left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

milenaveneziani commented Feb 10, 2017

Uh oh!

pwolfram commented Feb 10, 2017

Uh oh!

xylar commented Feb 10, 2017

Uh oh!

milenaveneziani commented Feb 10, 2017

Uh oh!

xylar commented Feb 11, 2017

Testing

Uh oh!

milenaveneziani commented Feb 11, 2017

Uh oh!

xylar commented Feb 15, 2017

Uh oh!

pwolfram left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

xylar commented Feb 8, 2017 •

edited

Loading

xylar commented Feb 8, 2017 •

edited

Loading