stan icon indicating copy to clipboard operation
stan copied to clipboard

Make const correct (accessors and mutators vs. API consistency)

Open SteveBronder opened this issue 5 years ago • 5 comments

Summary:

In a lot of mcmc we have get_* and set_* methods. I think it would be nice to just have const overloaded mutator and accessor methods. For instance with get_kappa(double x) we would have

inline double& kappa() { return kappa_;}
inline const double kappa() const { return kappa_;}

This gives us a standard API that I think looks nice and helps make things more const correct. But, there are methods which I think the compiler generates such as get_param_names. Unless we are okay with modifying some bits of the compiler we will end up with multiple styles of accessor. If we don't want that we can still apply const correct stuff to the setters and getters

Current Version:

v2.21.0

SteveBronder avatar Nov 06 '19 03:11 SteveBronder

@betanalpha Why do some of the setters have these hidden limits i.e.

  inline void set_t0(double t) {
    if (t > 0)
      t0_ = t;
  }

Should those be errors? warnings? I'd like to have things like that at least doc'd

SteveBronder avatar Nov 06 '19 06:11 SteveBronder

This code was written a long time ago but I believe I picked up the pattern from OO best practices I was reading at the time; the pattern of validating and then mutating only when the input is valid ensures that every object instance is always in a valid state. I just spent a few minutes googling around and I can't find a name for the pattern but it does show up in a bunch of code examples.

The challenge with adding warnings is that you'd have to pipe through a logger, and error-by-exception will require guarding all of the code that uses these methods with exception handling. I have no problem with either.

betanalpha avatar Nov 07 '19 01:11 betanalpha

Are these bounds in a paper somewhere? Like for the ones for learning stepsize adaptation?

https://github.com/stan-dev/stan/blob/736311d88e99b997f5b902409752fb29d6ec0def/src/stan/mcmc/stepsize_adaptation.hpp

Skimmed over your conceptual intro and geometric foundations paper and didn't see these bounds listed anywhere though would guess they are a technical detail that would be in a citation

SteveBronder avatar Nov 07 '19 02:11 SteveBronder

They’re relatively standard notation for dual averaging — Matt Hoffman covered them in the original NUTS paper. See Section 3.2.1 of http://jmlr.org/papers/volume15/hoffman14a/hoffman14a.pdf http://jmlr.org/papers/volume15/hoffman14a/hoffman14a.pdf.

On Nov 6, 2019, at 9:57 PM, Steve Bronder [email protected] wrote:

Are these bounds in a paper somewhere? Like for the ones for learning stepsize adaptation?

https://github.com/stan-dev/stan/blob/736311d88e99b997f5b902409752fb29d6ec0def/src/stan/mcmc/stepsize_adaptation.hpp https://github.com/stan-dev/stan/blob/736311d88e99b997f5b902409752fb29d6ec0def/src/stan/mcmc/stepsize_adaptation.hpp Skimmed over your conceptual intro and geometric foundations paper and didn't see these bounds listed anywhere though would guess they are a technical detail that would be in a citation

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/stan-dev/stan/issues/2849?email_source=notifications&email_token=AALU3FRFR4ODJSGZNKNKAU3QSN7Z7A5CNFSM4JJO2RK2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDIXOBQ#issuecomment-550598406, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALU3FTDQW5SQJLNNEGCGEDQSN7Z7ANCNFSM4JJO2RKQ.

betanalpha avatar Nov 07 '19 04:11 betanalpha

the pattern of validating and then mutating only when the input is valid ensures that every object instance is always in a valid state

Exactly. You lose the ability to keep things valid if you just hand out non-const references.

bob-carpenter avatar Nov 07 '19 20:11 bob-carpenter