Modify API for advanced settings. (FastTree, RandomForest) #2047

abgoswam · 2019-01-07T02:35:49Z

Towards #1798 . Also fixes #2019

This PR addresses only the FastTree and RandomForest algos. For the other remaining components, we will have separate PRs.

The following 3 changes have been made:

Two distinct extension methods, one for simple arguments and the other for advanced options
Make constructors internal
Rename Arguments to Options
Pass Options objects as arguments instead of Action delegate
Added Help text as the XML comment of Options
Rename Options objects as options (instead of args or advancedSettings used so far)

sfilipi · 2019-01-07T07:46:43Z

        public bool UnbalancedSets = false;

Might be good to put the Help text as the XML comment of the arguments. We didn't do it on the previous swap because there was half a thought for the Args classes to go away and get substituted with something else, like direct fields. #Resolved

Refers to: src/Microsoft.ML.FastTree/FastTreeArguments.cs:31 in b255eb3. [](commit_id = b255eb3, deletion_comment = False)

artidoro · 2019-01-07T10:08:36Z

src/Microsoft.ML.FastTree/FastTreeRegression.cs

+        public FastTreeRegressionTrainer(IHostEnvironment env,
+            string labelColumn,
+            string featureColumn,
+            string weightColumn,


These fields are also present in the Options object I believe. I think we should not have them as arguments in the constructor to eliminate the risk of them being set in two different places. #Resolved

This applies to the other tree based trainers.

In reply to: 245603313 [](ancestors = 245603313)

Thanks for pointing this out. I have updated the API signature to not have these.

In reply to: 245607972 [](ancestors = 245607972,245603313)

artidoro · 2019-01-07T10:23:05Z

@TomFinley @abgoswam
When inspecting the Options class for FastTreeRegressionTrainer, I see that it contains the following field: IDataView TrainData

It seems that this could lead to confusion, as we don't set training data in the arguments. Instead we pass it to the Fit() method. Correct me if I am wrong, but I think that this field is used by the entrypoints. How would we go about removing it from the Options class then? #Closed

yaeldekel · 2019-01-07T17:57:13Z

src/Microsoft.ML.FastTree/BoostingFastTree.cs

+            string featureColumn,
+            string weightColumn,
+            string groupIdColumn,
+            TArgs advancedSettings)


TArgs advancedSettings [](start = 12, length = 22)

Hi @abhishek Goswami, thanks for looking into this issue. Your change will also fix the problem described in issue #2019. #Resolved

Thanks for the info. Will reference #2019 in my PR so it gets closed upon merging this PR

In reply to: 245743197 [](ancestors = 245743197)

yaeldekel · 2019-01-07T17:59:58Z

                Args.LearningRates = learningRate;

This doesn't need to be inside the "if" anymore, since the base ctor does not change the value of Args.LearningRates. Reporting it in a channel is also not needed. #Resolved

Refers to: src/Microsoft.ML.FastTree/BoostingFastTree.cs:38 in b255eb3. [](commit_id = b255eb3, deletion_comment = False)

abgoswam · 2019-01-07T22:50:28Z

The observation makes sense. We should probably create a separate issue for this, so we investigate (a) why we have IDataView TrainData inside Options (b) usages (c) what it takes to remove etc.

I would keep it outside the purview of this PR, and handle it separately via a new issue

In reply to: 451888937 [](ancestors = 451888937)

abgoswam · 2019-01-08T00:08:07Z

        public bool UnbalancedSets = false;

fixed.

In reply to: 451848307 [](ancestors = 451848307)

Refers to: src/Microsoft.ML.FastTree/FastTreeArguments.cs:31 in b255eb3. [](commit_id = b255eb3, deletion_comment = False)

abgoswam · 2019-01-08T00:08:19Z

                Args.LearningRates = learningRate;

fixed

In reply to: 452024015 [](ancestors = 452024015)

Refers to: src/Microsoft.ML.FastTree/BoostingFastTree.cs:38 in b255eb3. [](commit_id = b255eb3, deletion_comment = False)

artidoro · 2019-01-08T11:30:36Z

src/Microsoft.ML.FastTree/FastTreeArguments.cs

        {
+            /// <summary>
+            /// Should we use derivatives optimized for unbalanced sets?
+            /// </summary>


Maybe for a summary description of an argument, it would be better to have a declarative sentence instead of a question. Could you change this to a declarative sentence? #Resolved

fixed

In reply to: 245963403 [](ancestors = 245963403)

artidoro · 2019-01-08T11:34:26Z

src/Microsoft.ML.FastTree/FastTreeArguments.cs

@@ -333,72 +447,130 @@ public abstract class BoostedTreeArgs : TreeArgs
        // REVIEW: TLC FR likes to call it bestStepRegressionTrees which might be more appropriate.
        //Use the second derivative for split gains (not just outputs). Use MaxTreeOutput to "clip" cases where the second derivative is too close to zero.
        //Turning BSR on makes larger steps in initial stages and converges to better results with fewer trees (though in the end, it asymptotes to the same results).
+        /// <summary>
+        /// Use best regression step trees?
+        /// </summary>


Same here if you could make this in a declarative sentence that would be better I think. #Resolved

fixed

In reply to: 245964319 [](ancestors = 245964319)

artidoro · 2019-01-08T11:35:48Z

src/Microsoft.ML.FastTree/FastTreeArguments.cs

        [Argument(ArgumentType.LastOccurenceWins, HelpText = "The learning rate", ShortName = "lr", SortOrder = 4)]
        [TGUI(Label = "Learning Rate", SuggestedSweeps = "0.025-0.4;log")]
        [TlcModule.SweepableFloatParamAttribute("LearningRates", 0.025f, 0.4f, isLogScale: true)]
        public Double LearningRates = Defaults.LearningRates;

+        /// <summary>
+        /// Shrinkage.
+        /// </summary>


Would be great to have more than just a single word to describe this. Maybe on the tlc documentation website there is more information on this parameter. #Pending

i guess its one of those known terms in Gradient Boosted trees. do u have any suggestions ?

In reply to: 245964690 [](ancestors = 245964690)

Maybe "The shrinkage". But yes I looked it up and you are right, so not necessary.

In reply to: 246105587 [](ancestors = 246105587,245964690)

artidoro · 2019-01-08T11:38:40Z

src/Microsoft.ML.FastTree/FastTreeClassification.cs

        {
            // Set the sigmoid parameter to the 2 * learning rate, for traditional FastTreeClassification loss
            _sigmoidParameter = 2.0 * Args.LearningRates;
        }

        /// <summary>
-        /// Initializes a new instance of <see cref="FastTreeBinaryClassificationTrainer"/> by using the legacy <see cref="Arguments"/> class.
+        /// Initializes a new instance of <see cref="FastTreeBinaryClassificationTrainer"/> by using the legacy <see cref="Options"/> class.


legacy [](start = 105, length = 6)

We need to change the comment, this is not legacy anymore. #Resolved

fixed

In reply to: 245965546 [](ancestors = 245965546)

artidoro · 2019-01-08T11:41:06Z

src/Microsoft.ML.FastTree/FastTreeRanking.cs

        {
            Host.CheckNonEmpty(groupIdColumn, nameof(groupIdColumn));
        }

        /// <summary>
-        /// Initializes a new instance of <see cref="FastTreeRankingTrainer"/> by using the legacy <see cref="Arguments"/> class.
+        /// Initializes a new instance of <see cref="FastTreeRankingTrainer"/> by using the legacy <see cref="Options"/> class.


legacy [](start = 92, length = 6)

Same here, this is not legacy now. #Resolved

fixed

In reply to: 245966171 [](ancestors = 245966171)

artidoro · 2019-01-08T11:41:52Z

src/Microsoft.ML.FastTree/FastTreeRegression.cs

        {
        }

        /// <summary>
-        /// Initializes a new instance of <see cref="FastTreeRegressionTrainer"/> by using the legacy <see cref="Arguments"/> class.
+        /// Initializes a new instance of <see cref="FastTreeRegressionTrainer"/> by using the legacy <see cref="Options"/> class.


legacy [](start = 95, length = 6)

Not legacy. #Resolved

fixed

In reply to: 245966414 [](ancestors = 245966414)

artidoro · 2019-01-08T11:45:04Z

In order to distinguish between the two constructors, would be great to add in summary documentation of the one that takes Options a few words explaining that this is the complete set of arguments including advanced settings. Something like: "Includes advanced options" (maybe use parameters, or settings instead of options). This also applies to the extensions to MLContext. #Resolved

artidoro · 2019-01-08T11:48:45Z

src/Microsoft.ML.FastTree/FastTreeClassification.cs

        /// </summary>
-        internal FastTreeBinaryClassificationTrainer(IHostEnvironment env, Arguments args)
+        public FastTreeBinaryClassificationTrainer(IHostEnvironment env, Options args)


args [](start = 81, length = 4)

Since we are renaming the Arguments class to Options, I think we should also change the parameter name to something more in line with this change. Maybe "options", or "settings". Maybe good idea to run this through Tom, or someone else to check. This applies to all the constructors that take the Options object. #Resolved

Renamed Options objects as options (instead of args or advancedSettings used so far in existing codebase)

In reply to: 245968100 [](ancestors = 245968100)

artidoro · 2019-01-08T11:50:36Z

src/Microsoft.ML.FastTree/FastTreeTweedie.cs

@@ -78,9 +76,9 @@ public sealed partial class FastTreeTweedieTrainer
        }

        /// <summary>
-        /// Initializes a new instance of <see cref="FastTreeTweedieTrainer"/> by using the legacy <see cref="Arguments"/> class.
+        /// Initializes a new instance of <see cref="FastTreeTweedieTrainer"/> by using the legacy <see cref="Options"/> class.


legacy [](start = 92, length = 6)

Change legacy. #Resolved

fixed

In reply to: 245968568 [](ancestors = 245968568)

artidoro · 2019-01-08T11:51:05Z

src/Microsoft.ML.FastTree/RandomForestClassification.cs

        {
            Host.CheckNonEmpty(labelColumn, nameof(labelColumn));
            Host.CheckNonEmpty(featureColumn, nameof(featureColumn));
        }

        /// <summary>
-        /// Initializes a new instance of <see cref="FastForestClassification"/> by using the legacy <see cref="Arguments"/> class.
+        /// Initializes a new instance of <see cref="FastForestClassification"/> by using the legacy <see cref="Options"/> class.


legacy [](start = 94, length = 6)

Remove legacy. #Resolved

fixed

In reply to: 245968731 [](ancestors = 245968731)

artidoro · 2019-01-08T11:51:31Z

src/Microsoft.ML.FastTree/RandomForestRegression.cs

        {
            Host.CheckNonEmpty(labelColumn, nameof(labelColumn));
            Host.CheckNonEmpty(featureColumn, nameof(featureColumn));
        }

        /// <summary>
-        /// Initializes a new instance of <see cref="FastForestRegression"/> by using the legacy <see cref="Arguments"/> class.
+        /// Initializes a new instance of <see cref="FastForestRegression"/> by using the legacy <see cref="Options"/> class.


legacy [](start = 90, length = 6)

Change legacy. #Resolved

fixed

In reply to: 245968841 [](ancestors = 245968841)

artidoro · 2019-01-08T11:53:59Z

src/Microsoft.ML.FastTree/TreeTrainersCatalog.cs

+        /// <param name="ctx">The <see cref="RegressionContext"/>.</param>
+        /// <param name="advancedSettings">Algorithm advanced settings.</param>
+        public static FastTreeRegressionTrainer FastTree(this RegressionContext.RegressionTrainers ctx,
+            FastTreeRegressionTrainer.Options advancedSettings)


advancedSettings [](start = 46, length = 16)

I agree that we should name this something different from arguments. However, now it does not only contain advanced settings, it also contains the names of the input columns, and some basic settings. Would be great if this name matched the name we give to the Options parameter in the public constructor of the trainers. This applies to all the extensions in this file. #Resolved

Using options consistently

In reply to: 245969481 [](ancestors = 245969481)

artidoro · 2019-01-08T11:56:44Z

Sounds good, this may be outside the scope of the PR. I suspect that it is easier than we think. Maybe it simply requires that field to be made internal in the new Options class. If that's the case, maybe we can still have that as part of this PR. I will ask Tom and update this thread.

In reply to: 452110380 [](ancestors = 452110380,451888937)

artidoro · 2019-01-08T12:04:39Z

src/Microsoft.ML.StaticPipe/TreeTrainersStatic.cs

+        /// </example>
+        public static Scalar<float> FastTree(this RegressionContext.RegressionTrainers ctx,
+            Scalar<float> label, Vector<float> features, Scalar<float> weights,
+            FastTreeRegressionTrainer.Options advancedSettings,


Same for static extensions, if we could rename the advancedSettings to something else it would be better I think. #Resolved

artidoro · 2019-01-08T12:06:04Z

src/Microsoft.ML.StaticPipe/TreeTrainersStatic.cs

+        /// </example>
+        public static Scalar<float> FastTree(this RegressionContext.RegressionTrainers ctx,
+            Scalar<float> label, Vector<float> features, Scalar<float> weights,
+            FastTreeRegressionTrainer.Options advancedSettings,


Is it possible to remove these fields? We discussed earlier that they are repeating some settings with the Options object. #Resolved

@TomFinley

In the API which accepts the Options argument, how do we handle the arguments label, features, weights in the Static API ?

In the corresponding Dynamic API, we were able to remove the parameters for labelColumn, featureColumn and weightColumn because these were already part of the Options class

In reply to: 245972452 [](ancestors = 245972452)

You obviously cannot remove the parameters for these here. The options object just accepts a string. The entire point of the statically typed API is that someone can define a pipeline in a way that is type safe. So, no, you are not removing these parameters.

In reply to: 246165575 [](ancestors = 246165575,245972452)

Thanks Tom for the clarification.

In reply to: 246170641 [](ancestors = 246170641,246165575,245972452)

artidoro · 2019-01-08T18:36:48Z

From talking with Tom, it seems that this is a problem that will require a separate PR!

In reply to: 452273298 [](ancestors = 452273298,452110380,451888937)

abgoswam · 2019-01-08T19:22:48Z

Currently the summary documentation for the constructor taking Options argument reads : Initializes a new instance of <see cref="FastTreeBinaryClassificationTrainer"/> by using the <see cref="Options"/> class.. The Options summary reads Algorithm advanced settings

I feel this is sufficient. Isn't it ?

Also, am following Tom's suggestion of having a single constructor

In reply to: 452270462 [](ancestors = 452270462)

…gs or advancedSettings used so far)

TomFinley · 2019-01-08T21:38:09Z

@TomFinley @abgoswam
When inspecting the Options class for FastTreeRegressionTrainer, I see that it contains the following field: IDataView TrainData

It seems that this could lead to confusion, as we don't set training data in the arguments. Instead we pass it to the Fit() method. Correct me if I am wrong, but I think that this field is used by the entrypoints. How would we go about removing it from the Options class then? #Closed

Anything relating to entry-points specifically should be internal, but we must do so carefully, since currently entry-points and command line processing relies on processing of fields which is currently done on public fields only, whereas it should be over public or internal fields. Once that is done, any fields that are relevant to entry-points only should be internal.

(This is a problem for all the settings objects, that they are exposing things that should not be in the public API in some cases. (Indeed entry-points will need a lot of work to make them work for estimators and transformers. Estimator graphs are sort of a "lightweight" estimator chain as they stand right now that performs absolutely no validation. But for now, anything relating to entry-points should just be hidden.) #Resolved

TomFinley · 2019-01-08T22:16:41Z

src/Microsoft.ML.FastTree/FastTreeClassification.cs

-            : base(env, args, TrainerUtils.MakeBoolScalarLabel(args.LabelColumn))
+        /// <param name="env">The instance of <see cref="IHostEnvironment"/>.</param>
+        /// <param name="options">Algorithm advanced settings.</param>
+        public FastTreeBinaryClassificationTrainer(IHostEnvironment env, Options options)


public FastTreeBinaryClassificationTrainer(IHostEnvironment env, Options options) [](start = 8, length = 81)

So, why add two constructors? Doesn't it suffice to just have the two overloads be on the MLContext extension methods? Shouldn't there just be one constructor? (Possibly an internal one, in which case you could avoid changing this altogether, except for the rename to options?) #Resolved

Created issue #2100 to track this.

For now I have made both the constructors internal.

In reply to: 246171294 [](ancestors = 246171294)

TomFinley · 2019-01-08T22:21:35Z

src/Microsoft.ML.FastTree/TreeTrainersCatalog.cs

        {
            Contracts.CheckValue(ctx, nameof(ctx));
            var env = CatalogUtils.GetEnvironment(ctx);
-            return new FastTreeRegressionTrainer(env, labelColumn, featureColumn, weights, numLeaves, numTrees, minDatapointsInLeaves, learningRate, advancedSettings);
+            return new FastTreeRegressionTrainer(env, labelColumn, featureColumn, weights, numLeaves, numTrees, minDatapointsInLeaves, learningRate);


FastTreeRegressionTrainer(env, labelColumn, featureColumn, weights, numLeaves, numTrees, minDatapointsInLeaves, learningRate); [](start = 23, length = 126)

So, this is kind of what I mean. While these two extension methods are good, I might have preferred that we just have a single constructor (always taking Options), and we just keep the flat constructor as simple as possible (e.g., take only the Options class, at least publicly). If there are "N" extension methods on this (which we consider the premier public surface!) there don't have to be a corresponding "N" constructors. This could have easily (and indeed more cleanly) acheived with exactly one constructor (again possibly internal) on the underlying object that just took an Options object, simply. As it is we're making the underlying object considerably more complex, I think, than it really has to be. It would be nice if the complexity was contained here. (I believe the intention is that, 99% of the time, people create their components through the MLContext and its subproperties. Let me know if this plan has changed or has met with resistance that leads us to reconsider.) #Resolved

Created issue #2100 to track this.

For now I have made the constructors internal.

In reply to: 246172562 [](ancestors = 246172562)

abgoswam · 2019-01-08T22:53:48Z

Thanks Tom for the explanation. Created a separate issue #2079 for this.

In reply to: 452460937 [](ancestors = 452460937)

artidoro

sfilipi

abgoswam added 4 commits January 7, 2019 01:28

Changes for FastTree & related learners

0c1ff77

Removing defaults from some of the newly added APIs

0d380ed

Argument -> Options

9c9c114

Pass objects as arguments instead of delegate

b255eb3

abgoswam changed the title ~~Modify API for advanced settings [FastTree*, RandomForest*]~~ Modify API for advanced settings. (FastTree, RandomForest) Jan 7, 2019

abgoswam requested review from TomFinley and artidoro January 7, 2019 02:37

artidoro reviewed Jan 7, 2019

View reviewed changes

yaeldekel reviewed Jan 7, 2019

View reviewed changes

review comments - 1

3341561

artidoro reviewed Jan 8, 2019

View reviewed changes

review comments - 2. updating comments, help summary etc

05018b8

abgoswam added 2 commits January 8, 2019 19:28

review comments - 3. Rename Options objects as options (instead of ar…

cb78dee

…gs or advancedSettings used so far)

update with latest master

7a445b2

TomFinley reviewed Jan 8, 2019

View reviewed changes

abgoswam force-pushed the abgoswam/action_arguments_2 branch from a8c56a0 to 7a445b2 Compare January 10, 2019 05:06

making the constructors internal

cd165e6

artidoro approved these changes Jan 10, 2019

View reviewed changes

sfilipi assigned abgoswam Jan 11, 2019

sfilipi added the API Issues pertaining the friendly API label Jan 11, 2019

sfilipi approved these changes Jan 11, 2019

View reviewed changes

abgoswam merged commit a927479 into dotnet:master Jan 11, 2019

abgoswam mentioned this pull request Jan 16, 2019

KMeans and Implicit weight cleanup #2158

Merged

wschin mentioned this pull request Jan 17, 2019

Number of feature columns #2179

Closed

abgoswam deleted the abgoswam/action_arguments_2 branch January 21, 2019 21:25

ghost locked as resolved and limited conversation to collaborators Mar 25, 2022

Modify API for advanced settings. (FastTree, RandomForest) #2047

Modify API for advanced settings. (FastTree, RandomForest) #2047

Conversation

abgoswam commented Jan 7, 2019 • edited Loading

sfilipi commented Jan 7, 2019 • edited by abgoswam Loading

artidoro Jan 7, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artidoro commented Jan 7, 2019 • edited Loading

yaeldekel Jan 7, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaeldekel commented Jan 7, 2019 • edited by abgoswam Loading

abgoswam commented Jan 7, 2019 • edited Loading

abgoswam commented Jan 8, 2019

abgoswam commented Jan 8, 2019

artidoro Jan 8, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artidoro Jan 8, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artidoro Jan 8, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artidoro Jan 8, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artidoro Jan 8, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artidoro Jan 8, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artidoro commented Jan 8, 2019 • edited by abgoswam Loading

artidoro Jan 8, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artidoro Jan 8, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artidoro Jan 8, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artidoro Jan 8, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artidoro Jan 8, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artidoro commented Jan 8, 2019

artidoro Jan 8, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

artidoro Jan 8, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

abgoswam Jan 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artidoro commented Jan 8, 2019

abgoswam commented Jan 8, 2019 • edited Loading

TomFinley commented Jan 8, 2019 • edited by abgoswam Loading

TomFinley Jan 8, 2019 • edited by artidoro Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomFinley Jan 8, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abgoswam commented Jan 8, 2019 • edited Loading

artidoro left a comment

Choose a reason for hiding this comment

sfilipi left a comment

Choose a reason for hiding this comment

abgoswam commented Jan 7, 2019 •

edited

Loading

sfilipi commented Jan 7, 2019 •

edited by abgoswam

Loading

artidoro Jan 7, 2019 •

edited by abgoswam

Loading

artidoro commented Jan 7, 2019 •

edited

Loading

yaeldekel Jan 7, 2019 •

edited by abgoswam

Loading

yaeldekel commented Jan 7, 2019 •

edited by abgoswam

Loading

abgoswam commented Jan 7, 2019 •

edited

Loading

artidoro Jan 8, 2019 •

edited by abgoswam

Loading

artidoro Jan 8, 2019 •

edited by abgoswam

Loading

artidoro Jan 8, 2019 •

edited by abgoswam

Loading

artidoro Jan 8, 2019 •

edited by abgoswam

Loading

artidoro Jan 8, 2019 •

edited by abgoswam

Loading

artidoro Jan 8, 2019 •

edited by abgoswam

Loading

artidoro commented Jan 8, 2019 •

edited by abgoswam

Loading

artidoro Jan 8, 2019 •

edited by abgoswam

Loading

artidoro Jan 8, 2019 •

edited by abgoswam

Loading

artidoro Jan 8, 2019 •

edited by abgoswam

Loading

artidoro Jan 8, 2019 •

edited by abgoswam

Loading

artidoro Jan 8, 2019 •

edited by abgoswam

Loading

artidoro Jan 8, 2019 •

edited by abgoswam

Loading

artidoro Jan 8, 2019 •

edited by abgoswam

Loading

abgoswam Jan 8, 2019 •

edited

Loading

abgoswam commented Jan 8, 2019 •

edited

Loading

TomFinley commented Jan 8, 2019 •

edited by abgoswam

Loading

TomFinley Jan 8, 2019 •

edited by artidoro

Loading

TomFinley Jan 8, 2019 •

edited by abgoswam

Loading

abgoswam commented Jan 8, 2019 •

edited

Loading