Feature/issue 1062 ode speedup #1066

wds15 · 2018-11-17T09:54:28Z

Ok, I found a way which I think is totally fine with our AD logic. We are still getting a nice 18% or so speedup on the sir example.

Summary

The intent of the PR is to speedup the ODE integration. The approach is to avoid repeated allocation of the parameters vector theta_ on the nested AD stack. The nested AD is carried out when calling the coupled_ode_system functor. Since the parameters will never change during ODE integration we do not need to have these on the nested AD stack. The speedup of the SIR performance benchmark when running in 5 repetitions is 19%:

develop: stat_comp_benchmarks/benchmarks/sir/sir.stan,98.8624837399
this PR: stat_comp_benchmarks/benchmarks/sir/sir.stan,80.3885923386

[10:11:37][sebi@sebastians-macbook-pro-1:~/work/performance-tests-cmdstan]$ ./comparePerformance.py performance.csv develop_performance.csv
('stat_comp_benchmarks/benchmarks/sir/sir.stan', 0.81)

Tests

test/unit/math/rev/mat/functor/integrate_ode_adams_prim_test.cpp
test/unit/math/rev/mat/functor/integrate_ode_adams_rev_test.cpp
test/unit/math/rev/mat/functor/integrate_ode_bdf_rev_test.cpp
test/unit/math/rev/mat/functor/integrate_ode_cvodes_grad_rev_test.cpp
test/unit/math/rev/mat/functor/integrate_ode_bdf_prim_test.cpp
test/unit/math/rev/arr/functor/integrate_ode_rk45_grad_test.cpp
test/unit/math/rev/arr/functor/integrate_ode_rk45_tooMuchWork_test.cpp
test/unit/math/rev/arr/functor/integrate_ode_rk45_test.cpp
test/unit/math/prim/arr/functor/integrate_ode_rk45_test.cpp
test/unit/math/rev/arr/functor/coupled_ode_system_test.cpp
test/unit/math/prim/arr/functor/coupled_ode_system_test.cpp

Side Effects

Faster ODE integration.

Checklist

Math issue make coupled_ode_system more efficient #1062
Copyright holder: Sebastian Weber

The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
- Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
- Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)
the basic tests are passing
- unit tests pass (to run, use: ./runTests.py test/unit)
- header checks pass, (make test-headers)
- docs build, (make doxygen)
- code passes the built in C++ standards checks (make cpplint)
the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested (no behavior as tested so far is changed)

…-ode-speedup

…y having a copy of it on the outer stack

…n cvodes solvers

wds15 · 2019-01-27T16:42:41Z

Bump. Once tests finish this can be reviewed and merged I think.

syclik · 2019-01-28T15:24:27Z

stan/math/rev/arr/functor/coupled_ode_system.hpp

@@ -72,7 +72,7 @@ struct coupled_ode_system<F, double, var> {
      : f_(f),
        y0_dbl_(y0),
        theta_(theta),
-        theta_dbl_(value_of(theta)),
+        theta_copy_(theta),


I discussed with @wds15. This is not the right copy; we need to create a copy that is disconnected from theta and is on our no_chain stack.

Please test that there's nothing added to the stack on construction.

Ok, I am using the nonstack thing now (really ugly to get things there!). However, I am not sure how to test and why to test that nothing gets on construction onto the stack. Ok, I could use ChainableStack::instance().var_stack.size()... but that is a not an exported API and is used nowhere in any test which I could find such that I am not sure that this can be tested given our API. If you think it is important to test, can you suggest a scheme to do so?

Never mind... I figured that we can test the stack size changes if we nest things.

... this is one of the annoyances of our nesting system - things are slightly inconsistent wrt to the non-nested stack; at least to me.

So the coupled_ode_system is now tested to not put stuff on the AD stack upon construction.

You're in the guts of the autodiff stack and you're playing with things that live globally and within the nested stack. You should be testing that what you're doing is consistent with what you think should be happening. Don't just assume the code you've written works... that will get us into a lot of trouble.

Also, since we're now dealing with threading and mpi and gpu and all sorts of other complications that could be unsafe, it makes a lot of sense to make sure these unit tests actually trigger failures if the basic assumptions of validity of autodiff stack through nested operations falls apart.

stan/math/rev/arr/functor/coupled_ode_system.hpp

…gs/RELEASE_500/final)

syclik

Cool! I think it's the doc that needs to be updated. But I think the changes are pretty good.

syclik · 2019-02-02T04:28:56Z

stan/math/rev/arr/functor/coupled_ode_system.hpp

@@ -45,7 +45,7 @@ struct coupled_ode_system<F, double, var> {
  const F& f_;
  const std::vector<double>& y0_dbl_;
  const std::vector<var>& theta_;
-  const std::vector<double> theta_dbl_;
+  std::vector<var> theta_copy_;


Could we please rename this to something descriptive? This is meant to be a copy of the theta, but placed on the global stack, but essentially treated as temporaries where we can reset the adjoints. I don't know what a good name is, but just calling it theta_copy_ really isn't that great.

The old name was ok because it implies we've lost the connections between theta_ and theta_dbl_ by demoting it to a double.

syclik · 2019-02-02T04:31:57Z

stan/math/rev/arr/functor/coupled_ode_system.hpp

@@ -45,7 +45,7 @@ struct coupled_ode_system<F, double, var> {
  const F& f_;
  const std::vector<double>& y0_dbl_;
  const std::vector<var>& theta_;
-  const std::vector<double> theta_dbl_;
+  std::vector<var> theta_copy_;


(For the top-level comment... I can't comment through the PR interface on lines of code that aren't really affected.)

Can you document what this implementation does? I think it'd help anyone else trying to maintain the code.

that the theta is copied and put on the stack, but are not meant to be autodiffed

that there's use of nested autodiff to compute the sensitivities

syclik · 2019-02-02T04:37:31Z

stan/math/rev/arr/functor/coupled_ode_system.hpp

-        msgs_(msgs) {}
+        msgs_(msgs) {
+    for (auto& p : theta)
+      theta_copy_.emplace_back(var(new vari(value_of(p), false)));


If we know p is a stan::math::var, why not use v.val() directly? I think it'd be a little clearer.

To me the value_of is actually clearer as I don't need to recal the details of the var class. Calling value_of always gives me a double - no matter what. The val method feels like an internal function whereas the value_of is a clear API.

I'll try to explain why I find this so much more confusing than if this was:

for (var&& p : theta) theta_copy_.emplace_back(var(new vari(p.val(), false));

And this is coming from me having to read this code to figure it out.

value_of() is a function that's designed to work on double, var, fvar<double>, fvar<var>, and fvar<fvar<var>>. So I'm looking at value_of(p) and trying to figure out... "what is p?"

auto& p instead of var&& p doesn't help me at all. I know that p is one of theta, so theta is a collection, but what is it? Since I'm flipping between base template classes that can handle all autodiff types, it's not always easy to keep track that this is a rev only implementation. It'd really just help to have this be var&& p so the type is obvious.

theta... now I need to go and find where this is defined. C++ is a structured language, but it actually allows the declaration of struct and class member variables either at the top, below, or anywhere in between. And now I've got to search. I was looking at this using the GitHub interface, so the actual declaration was off of the interface; in order for me to go look at this, I had to go and check out the branch, open the file, all to find that theta is declared as std::vector<var>.

I think the thing that we're not seeing eye-to-eye on is the use of value_of. I find it much easier to reason about something that's only valid for var instead of having to work to figure out whether the object passed in is one of many different types. (It's just a little more mental work the reader has to do when it can all be made explicit as an alternative.)

(By the way, your statement is incorrect -- value_of does not always give a double. It'll give the type of the value, which for fvar<var> will be var.)

Hopefully you've gotten some context on why this is harder to read for a reviewer or anyone else coming to the code later on. This is minor, but the idea still stands -- make it easier for the next person to read. It takes effort now, but it's so worth it later on.

Oh... value_of is defined differently in my head... I always forget about higher order things. With the correct definitions the val call is better, yeah. Sorry, but I am not used to meaning of value_of.

Honestly, I do expect the reviewer to look at the file and not just the diff from git. It is really hard to expect our changes to be self-explanatory on a single diff line. The auto vs actual type ... I read the opposite elsewhere. Your third point I do not quite understand. I am looping over theta which is the argument to the constructor.

EDIT: The compiler only accepts

for (const var& p : theta)

So I went with that.

Thanks! The third point wasn't actually there... you're right. I should have looked at the constructor signature, not the actual type of the variable.

stan/math/rev/core/var.hpp

stan/math/rev/arr/functor/coupled_ode_system.hpp

…tan-dev/math into feature/issue-1062-ode-speedup

…ling

charlesm93 · 2019-02-20T22:41:49Z

I can hop in and complete the review. I'll do a review of the code and make sure all of @syclik 's requests have been addressed.

charlesm93 · 2019-02-21T00:15:06Z

I discussed with @wds15. This is not the right copy; we need to create a copy that is disconnected from theta and is on our no_chain stack.

I get the basic idea based on the PR description but I'm not sure what the glitch was here. This makes me worry I'm missing important subtleties. @wds15 maybe we can discuss over a quick video call.

charlesm93

Most of the queries from the previous review have been addressed, with a minor exception, see line 133 of coupled_ode_system.hpp. The unit tests look fine, in particular they check that the size of the stack does not change. See my additional comments below.

stan/math/rev/mat/functor/cvodes_ode_data.hpp

stan/math/rev/arr/functor/coupled_ode_system.hpp

…gs/RELEASE_500/final)

wds15 · 2019-02-21T08:34:41Z

@charlesm93 The idea is to have the parameters on a stack which is disconnected to overall AD tree. Such a place is the nonstack AD tape which I never knew before talking to @syclik about it (this explains the comment you quote).

I added some comments as you requested in the code. Maybe we shortly discuss this at the meeting later in case you have more questions?

…tan-dev/math into feature/issue-1062-ode-speedup

charlesm93 · 2019-02-22T17:07:05Z

Ok, this all looks good to me. The only minor point missing is an explanatory comment on line 133 of coupled_ode_system.hpp. Once this is done, we should be good to go.

syclik

@charlesm93, thanks for following up on this!

It just dawned on me that we could have nest a couple times. I believe this PR will add a copy of the variables on the stack when we could easily just remove them when we leave the function. Your call on whether that makes sense. Thanks again. (and thanks, @wds15, for putting this together!)

stan/math/rev/arr/functor/coupled_ode_system.hpp

…work to straighten the decouple operation

…gs/RELEASE_500/final)

wds15 · 2019-04-01T08:24:50Z

Ok. So here is an approach which can work and has the best of both approaches. The idea is:

in the constructor of coupled_ode_system we call start_nested and then put the theta onto the nested no chain AD stack.
you would expect to put the recover_memory_nested into the destructor of coupled_ode_system, but that will not work as then the decoupling operation places things on the nested stack which gets cleared away.

Right now I address the 2. thing by doing the recover_memory_nested in the decoupled states operation.... which should not stay like this. So if this approach has merits to you, then we should turn the decouple_states operation into a static function of coupled_ode_system and call it after getting rid of the coupled_ode_system instance. This could work and give us

full independence of the outer AD tree
nothing left behind

If that makes sense to you, I would refactor accordingly.

(I still think that the approach to leave things behind on the no chain outer AD stack is cleanest and the best compromise... but you feel obvious strongly against about that while I am strongly against modifying the outer AD tree given we head into a parallel world)

syclik · 2019-04-01T12:53:43Z

(I still think that the approach to leave things behind on the no chain outer AD stack is cleanest and the best compromise... but you feel obvious strongly against about that while I am strongly against modifying the outer AD tree given we head into a parallel world)

I'm not strongly opposed to the approach of leaving things behind on the AD stack. It is a design decision and I think that's probably the cleanest and we should go with that. I think that should be the safest thing to do given the circumstances.

In general, we shouldn't just leave things behind. If we do, we should clearly state what's going on and why we've done it this way. That's what I feel strongly about, not the fact that we do it. By questioning whether we can remove it, we've actually come a long way in the design discussion that we wouldn't have.

Are you ok with going back to putting things on the global stack on construction? We can't remove them on destruction. That means the constructor and the functor are the functions that need to be locked, but the AD stack can be modified outside of those times without a problem.

wds15 · 2019-04-01T13:15:05Z

Great. I much prefer to go back to the design which uses the no chain approach (but leave things behind).

I will revert the code and add some doc around that as to what is safe / what not / what the side-effect is.

At least in my head having the guarantee that starting a nested AD tree gives me an independent sub-tree is a very good one (I think @bob-carpenter said this once if I am not wrong). I would expect that this holds right now in Stan-math - and we should keep it like this.

bob-carpenter · 2019-04-01T14:43:40Z

On Apr 1, 2019, at 9:15 AM, wds15 ***@***.***> wrote: Great. I much prefer to go back to the design which uses the no chain approach (but leave things behind). I will revert the code and add some doc around that as to what is safe / what not / what the side-effect is. At least in my head having the guarantee that starting a nested AD tree gives me an independent sub-tree is a very good one (I think @bob-carpenter said this once if I am not wrong).

That was certainly the intention.

I would expect that this holds right now in Stan-math - and we should keep it like this.

I agree.

…tan-dev/math into feature/issue-1062-ode-speedup

…-ode-speedup

…tan-dev/math into feature/issue-1062-ode-speedup

wds15 · 2019-05-05T13:11:01Z

I reverted to the make-nochain-copy approach described above which we agreed on. I feel much safer with this!

The only side-effect of this approach is that instances of the parameter vector theta will be left on the nochain AD stack (even after the coupled_ode_system instance is gone). Other than that I don't see how anything can break this approach in a parallel world where we are heading.

I added a note to the respective specialization which do this trick which explains what is done and what side-effects are. So this is ready for final review.

Tagging @syclik and @charlesm93 who are well familiar with the context here.

wds15 · 2019-05-08T12:02:38Z

Here we seem to have the same as issue as in #1072 such that I suggest the same here:

Either we can commit to a timeline, look for other reviewers or we drops this PR.

Tagging again @seantalts and @bob-carpenter for suggestions on this one.

syclik

Just have a couple of questions. It'd be great to address them just to have some answers, but the PR is great!

syclik · 2019-05-08T18:44:20Z

stan/math/prim/arr/functor/integrate_ode_rk45.hpp

@@ -44,6 +44,13 @@ namespace math {
 * method</a> as implemented in Boost's <code>
 * boost::numeric::odeint::runge_kutta_dopri5</code> integrator.
 *
+ * During ODE integration the global autodiff tape is continuously


What is meant by "the adjoints of the parameter vector are used for Jacobian calculations"? And how does that interact with concurrency?

test/unit/math/rev/arr/functor/coupled_ode_system_test.cpp

…tan-dev/math into feature/issue-1062-ode-speedup

…arer

…gs/RELEASE_500/final)

wds15 · 2019-05-09T09:40:13Z

@syclik : The confusing doc you found leading to your question

What is meant by "the adjoints of the parameter vector are used for Jacobian calculations"? And how does that interact with concurrency?

Is still a left-over from the old approach which used the outer AD tape. The approach which we implement now using the no chain stack is completely safe wrt to concurrency in the usual sense. I removed that comment there from the doc (which was also put into the cvodes_integrator file).

Once passes tests this should then be fine to merge.

Looks like there is once more the need for your approval. Sorry for that (I haven't actively dismissed your approval; probably that was automatic due to the change set).

Thanks!

(BTW, with the forthcoming independent AD in the parallel design docs we will be able to write this optimisation in a way such that nothing will be left behind, I think ... but that will still take some time until it lands, of course)

syclik · 2019-05-09T15:07:01Z

Thanks for removing that comment -- I was scratching my head at that for a little bit, but I'm glad it was cleaned up.

Yes, the way the approvals work now, it needs to be re-approved once a commit has been made. It's a pessimistic view of the world, but it's to prevent someone maliciously committing whatever they want after getting an approval and merging that. I think it's the right approach to take for an open-source project like ours.

syclik

Thanks!

wds15 · 2019-05-09T15:18:27Z

It makes sense to re-approve if there are changes...no objection to that. I just wasn't aware of it and always feel guilty to use peoples time if not actually needed.

wds15 · 2019-05-10T06:25:52Z

Whow! 6 month PR being around / 4 month under review or so / finally merged. Cool.

I know this PR bends our usual conventions... but it is worth it and at least I learned a lot about our AD reverse mode things.

Thanks @syclik for bearing with me on this one!

mattfidler · 2020-09-29T19:13:34Z

@wds15 doesn't this imply that time-varying covariates (ie. thetas that change) won't work correctly?

Sebastian Weber added 4 commits November 7, 2018 21:14

tune access to AD stack during ODE integration of the coupled system

877d5c3

make all tests happy

72922fe

recover adjoints of outer AD tree

3709f55

more lazy cleanup

f4046a7

wds15 changed the title ~~Feature/issue 1062 ode speedup~~ WIP Feature/issue 1062 ode speedup Jan 7, 2019

wds15 added 3 commits January 27, 2019 14:37

Merge remote-tracking branch 'origin/develop' into feature/issue-1062…

223885f

…-ode-speedup

avoid re-allocation of constant parameter vector on nested AD stack b…

017948c

…y having a copy of it on the outer stack

avoid a few copies and add explicit move semantics where appropiate i…

733f9af

…n cvodes solvers

wds15 changed the title ~~WIP Feature/issue 1062 ode speedup~~ Feature/issue 1062 ode speedup Jan 27, 2019

syclik reviewed Jan 28, 2019

View reviewed changes

stan/math/rev/arr/functor/coupled_ode_system.hpp Show resolved Hide resolved

wds15 and others added 3 commits January 28, 2019 21:26

move local theta copy to nonstack tape

2c8856f

add tests for constant stack size when instantiating coupled_ode_system

9dbd0f9

[Jenkins] auto-formatting by clang-format version 5.0.0-3~16.04.1 (ta…

4d22c0c

…gs/RELEASE_500/final)

syclik requested changes Feb 2, 2019

View reviewed changes

wds15 added 3 commits February 3, 2019 18:10

Merge branch 'feature/issue-1062-ode-speedup' of https://github.com/s…

965ab13

…tan-dev/math into feature/issue-1062-ode-speedup

rename theta_copy_ to theta_nochain_ and add doc for the special hand…

b8c64e7

…ling

address review comments

718804c

charlesm93 self-requested a review February 20, 2019 22:39

charlesm93 reviewed Feb 21, 2019

View reviewed changes

stan/math/rev/mat/functor/cvodes_ode_data.hpp Show resolved Hide resolved

stan/math/rev/mat/functor/cvodes_ode_data.hpp Show resolved Hide resolved

stan/math/rev/arr/functor/coupled_ode_system.hpp Show resolved Hide resolved

weberse2 and others added 3 commits February 21, 2019 09:01

add a comment to explain "manual" adjoint zeroing

f1d3c03

Merge commit 'd7dc42dee65a1f08ef9c2195ba940531e2e797a6' into HEAD

2561e04

[Jenkins] auto-formatting by clang-format version 5.0.0-3~16.04.1 (ta…

29b91dc

…gs/RELEASE_500/final)

Merge branch 'feature/issue-1062-ode-speedup' of https://github.com/s…

7715209

…tan-dev/math into feature/issue-1062-ode-speedup

syclik reviewed Feb 22, 2019

View reviewed changes

stan/math/rev/arr/functor/coupled_ode_system.hpp Show resolved Hide resolved

weberse2 and others added 3 commits April 1, 2019 10:17

put in approach which does not modify the outer AD tree; still needs …

2a365db

…work to straighten the decouple operation

Merge commit 'cb204e0bf0735a294c2689fc8bb3a4572a1becee' into HEAD

03c7a5e

[Jenkins] auto-formatting by clang-format version 5.0.0-3~16.04.1 (ta…

1e78847

…gs/RELEASE_500/final)

weberse2 and others added 5 commits April 2, 2019 20:37

Merge branch 'feature/issue-1062-ode-speedup' of https://github.com/s…

b5053c1

…tan-dev/math into feature/issue-1062-ode-speedup

Merge remote-tracking branch 'origin/develop' into feature/issue-1062…

83ca05d

…-ode-speedup

revert to the nochain approach

e506db8

doc special nochain speedup side-effects

c0fb42c

Merge branch 'feature/issue-1062-ode-speedup' of https://github.com/s…

f07855b

…tan-dev/math into feature/issue-1062-ode-speedup

syclik previously approved these changes May 8, 2019

View reviewed changes

weberse2 added 3 commits May 9, 2019 11:19

Merge branch 'feature/issue-1062-ode-speedup' of https://github.com/s…

2390317

…tan-dev/math into feature/issue-1062-ode-speedup

remove left-over outdated doc

20883c6

address reviewer comments on test; make test-purpose for the size cle…

f71715f

…arer

wds15 dismissed syclik’s stale review via f71715f May 9, 2019 09:28

yashikno and others added 2 commits May 9, 2019 09:34

Merge commit '57215ead04f95c25cb36b7b8f21776d0090cff55' into HEAD

cc72c41

[Jenkins] auto-formatting by clang-format version 5.0.0-3~16.04.1 (ta…

fd7846f

…gs/RELEASE_500/final)

syclik approved these changes May 9, 2019

View reviewed changes

wds15 merged commit 0784a82 into develop May 10, 2019

wds15 mentioned this pull request May 10, 2019

make coupled_ode_system more efficient #1062

Closed

wds15 deleted the feature/issue-1062-ode-speedup branch June 30, 2019 17:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/issue 1062 ode speedup #1066

Feature/issue 1062 ode speedup #1066

wds15 commented Nov 17, 2018 •

edited

Loading

wds15 commented Jan 27, 2019

syclik Jan 28, 2019

syclik Jan 28, 2019

wds15 Jan 28, 2019

wds15 Jan 29, 2019

syclik Feb 2, 2019

syclik left a comment

syclik Feb 2, 2019

syclik Feb 2, 2019

syclik Feb 2, 2019

wds15 Feb 3, 2019

syclik Feb 4, 2019

wds15 Feb 4, 2019 •

edited

Loading

syclik Feb 25, 2019

charlesm93 commented Feb 20, 2019

charlesm93 commented Feb 21, 2019

charlesm93 left a comment •

edited

Loading

wds15 commented Feb 21, 2019

charlesm93 commented Feb 22, 2019 •

edited

Loading

syclik left a comment

wds15 commented Apr 1, 2019

syclik commented Apr 1, 2019

wds15 commented Apr 1, 2019

bob-carpenter commented Apr 1, 2019 via email

wds15 commented May 5, 2019

wds15 commented May 8, 2019

syclik left a comment

syclik May 8, 2019

wds15 commented May 9, 2019

syclik commented May 9, 2019

syclik left a comment

wds15 commented May 9, 2019

wds15 commented May 10, 2019

mattfidler commented Sep 29, 2020

Feature/issue 1062 ode speedup #1066

Feature/issue 1062 ode speedup #1066

Conversation

wds15 commented Nov 17, 2018 • edited Loading

Summary

Tests

Side Effects

Checklist

wds15 commented Jan 27, 2019

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

syclik 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

wds15 Feb 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charlesm93 commented Feb 20, 2019

charlesm93 commented Feb 21, 2019

charlesm93 left a comment • edited Loading

Choose a reason for hiding this comment

wds15 commented Feb 21, 2019

charlesm93 commented Feb 22, 2019 • edited Loading

syclik left a comment

Choose a reason for hiding this comment

wds15 commented Apr 1, 2019

syclik commented Apr 1, 2019

wds15 commented Apr 1, 2019

bob-carpenter commented Apr 1, 2019 via email

wds15 commented May 5, 2019

wds15 commented May 8, 2019

syclik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wds15 commented May 9, 2019

syclik commented May 9, 2019

syclik left a comment

Choose a reason for hiding this comment

wds15 commented May 9, 2019

wds15 commented May 10, 2019

mattfidler commented Sep 29, 2020

wds15 commented Nov 17, 2018 •

edited

Loading

wds15 Feb 4, 2019 •

edited

Loading

charlesm93 left a comment •

edited

Loading

charlesm93 commented Feb 22, 2019 •

edited

Loading