stan-dev / cmdstanpy

CmdStanPy is a lightweight interface to Stan for Python users which provides the necessary objects and functions to compile a Stan program and fit the model to data using CmdStan.
BSD 3-Clause "New" or "Revised" License
149 stars 67 forks source link

`iter_warmpup=0` and `fixed_param=True` will always fail for stan 2.34.1 #766

Open carrascomj opened 6 days ago

carrascomj commented 6 days ago

Summary:

If fixed_param=True, adapt_engaged will always be left as default (True) because of this early return https://github.com/stan-dev/cmdstanpy/blob/develop/cmdstanpy/cmdstan_args.py#L361.

This is okay for the newer stan since this if-statement. But for previous stan (tried 2.34.1), this will always fail since adapt_engaged must be False even when fixed_param is supplied.

Description:

This only affects previous versions of Stan. I am not sure about the policy of supporting versions but it is a breaking change that will make previously working code fail if cmdstan itself is not updated.

bob-carpenter commented 5 days ago

Thanks for reporting. Sounds like a bug we should fix.

WardBrian commented 2 days ago

@carrascomj what action would you like to see, given that the latest cmdstan fixed this issue?

carrascomj commented 2 days ago

Removing that early return and testing that it satisfies the expectations would make sense. In general, (only in my opinion) silently removing choices of the user is bound to cause troubles; if the API changes in stan again, it will be very difficult to catch this kind of errors that affect only particular combinations of command line options.

I pointed out in a different repository (which uses cmdstanpy https://github.com/biosustain/Maud/issues/469#issuecomment-2210892737) that a particular combination of parameters makes adapt_delta never set to True (latest Stan and cmdstanpy). I will try to repro and record exactly when it happens and come back to this.

As a nice-to-have as an user, it would also be convenient to have a clear statement of Minimum Required Version of Stan supported by the particular version of cmdstanpy. I see that in CI, you test for the latest stan commit, so I guess the policy is to only support the latest Stan (?).

WardBrian commented 2 days ago

We test against the latest released version, but we currently support old versions on a best-effort basis, which can be seen by the presence of version-based conditionals in the code.

One item under consideration for the upcoming 2.0 release is to define a rolling window where the previous N cmdstan releases are officially supported/tested, and the rest are explicitly dropped, but at the moment there is not any explicit policy

carrascomj commented 2 days ago

Thanks for the explanation. I believe the rolling window is a great idea, although I understand that is quite a huge effort of tinkering with github actions, I'll be looking forward to that!

WardBrian commented 2 days ago

Removing that early return and testing that it satisfies the expectations would make sense.

Removing the early return seems fine at first glance, since the validate function already should prevent illegal configurations. It's difficult to directly test that it fixes the issue on older versions, for the reasons mentioned above.