Feature/issue 2670 warmup stepsize factor

[Tuxonomics]

Submission Checklist

• Run unit tests: ./runTests.py src/test/unit
• Run cpplint: make cpplint
• Declare copyright holder and open-source license: see below

Summary

As in issue 2670.
The stepsize factor is exposed in the services via function overload of the existing algorithms.

Intended Effect

How to Verify

Side Effects

Documentation

Copyright and Licensing

Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company): Jonas Kose

By submitting this pull request, the copyright holder is agreeing to 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/)

[Tuxonomics]

@bob-carpenter I added a unit test for the getter and setter of the new member in the stepsize_adaptation class. Regarding the services, the current unit tests are testing also the changes to the code.

[bob-carpenter]

I'm not sure why you're pinging me. @betanalpha would be better to review this one.

[bob-carpenter]

Also, we can't take anythnig without copyright assignment. You need to declare that you (or your assignee, such as a university), own the code and license it under the BSD.

[Tuxonomics]

Sorry, I didn't know who to ping. I pinged you because you accepted the issue, no more thought went into it.

@betanalpha Could you have a look at this? (PR regarding https://discourse.mc-stan.org/t/issue-with-dual-averaging/5995/27)

[bob-carpenter]

I asked @betanalpha who's busy most of this week but can get to it next week. If that doesn't work, I can review it next week.

Also, just a heads up that this PR shouldn't change any adaptation strategies. If we change existing behavior, we'll want to test them thoroughly for performance and include them in a separate PR.

[Tuxonomics]

@bob-carpenter This PR keeps all the defaults. It only allows to set the coefficient in the assignment mu = log( mu_c * epsilon ). By default the coefficient is still 10.0.

I don't mind waiting for @betanalpha, but I would postpone working on an RStan PR until then.

[bob-carpenter]

You should be able to build on top of the branch. We want to expose this, so we'll make sure we can get it working.

[Tuxonomics]

Yes, I know. But I would like to avoid unnecessary refactoring. Once this PR here is cleared, the RStan changes should be easy too.

[betanalpha]

A few comments.

I think that the name mu_c should be changed. mu_c isn't on the same scale as mu, as it's the logarithm of mu_c that actually translates mu. Indeed it seems like expanding the log would be more interpretable, in which case we'd write

this->stepsize_adaptation_.set_mu(  log(this->nom_epsilon_) 
                                                           + this->stepsize_adaptation_.get_delta_mu());

with delta_mu defaulting to log(10).

We could keep the current logic but then any meaningful naming would be a bit unwieldy. Maybe log_mu_eps_scale?

The other big thing is that we shouldn't overload the service routes with a version that sets this new parameter and one that doesn't. There are already too many service routes to maintain and no where else do we allow parameters to be optionally set. We'll need to keep the expanded routes and then update all of the interfaces at the same time. @seantalts, do we have a well-established procedure for this yet, or would this be optimal after the move to the monorepo?

[bob-carpenter]

This is why we need to move to a builder-like pattern here with defaults for config so changes like this can be made without disrupting the interfaces. I'll have a design for that soon.

@seantalts, do we have a well-established procedure for this yet, or would this be optimal after the move to the monorepo?

The preferred way forward is to implement both, deprecate the old one, and then when the interfaces catch up, remove the deprecated one. It's a bit of a pain, but it's easier than manually finagling the continuous integration testing---we just don't have multiple platforms, so it's disruptive.

I'm afraid the monorepo won't help as that's only going to be math/stan/cmdstan, not rstan and pystan. The good news is that it looks like that's about to happen.

[Tuxonomics]

We could keep the current logic but then any meaningful naming would be a bit unwieldy. Maybe log_mu_eps_scale?

I will change the name.

The other big thing is that we shouldn't overload the service routes with a version that sets this new parameter and one that doesn't.

I agree that an overload is not a good solution. I used it because it was already in place.

The preferred way forward is to implement both, deprecate the old one, and then when the interfaces catch up, remove the deprecated one.

What does that mean for this PR now, also regarding the deprecation? Do I just change the variable name and then come back in a few months to clean up the services?

[bob-carpenter]

I think overloading is OK here to maintain backward compatibility. And yes, we'll clean up the ones that aren't used in Stan 3 at the latest, but we can also do it before then if PyStan and RStan and CmdStan all catch up to the new service code.

[betanalpha]

What we typically do in this circumstance is create corresponding forks of the three main interfaces (PyStan, RStan, and CmdStan) whose submodules point to the forked Stan repository. Once those forked interfaces are ready then all four can be merged to the corresponding develop branches in quick succession. It’s a pain, but something we occasionally have to do so we have some precedent procedure for the dev ops once the interface forks have been reviewed. It also avoids adding a bunch of unused routes that clutter up the Stan API and make adding similar features in the future even more of a pain.

…

On Oct 27, 2018, at 7:42 PM, Bob Carpenter ***@***.***> wrote: I think overloading is OK here to maintain backward compatibility. And yes, we'll clean up the ones that aren't used in Stan 3 at the latest, but we can also do it before then if PyStan and RStan and CmdStan all catch up to the new service code. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2675 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABdNlr7CCf3MVfLA4kRMMcyd7k3rYymlks5upO9cgaJpZM4XxWFs>.

[bob-carpenter]

Modifying Stan will cause the upstream tests to fail unless any new arguments get defaults (which forces them to be the last arguments). Typically, the dev ops needs to be finagled to connect the appropriate upstream version to the downstream tests. The bigger question is whether we should allow changes at the Stan level that are *not* reflected with matching PRs in PyStan, RStan, and CmdStan. Requiring four PRs across four repos in three languages is a bit of a hurdle for most devs (including me).

[Tuxonomics]

I renamed the variable. The name speaks for itself though I think it is a bit too verbose. It makes the code less readable.

Now I would just like to know how to proceed.

In general, the current function overloads will not break the interfaces. The users will just not be able to set this new parameter. Overall I'd also prefer a more structured approach, but should that now mean that I wait for the new builder, or can we proceed with this PR here?

[rok-cesnovar]

Closing stale stan-dev/stan pull requests. This one has been stale for 2+ years and has a lot of unresolved conflicts. Feel free to reopen if you wish to continue on this.