Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
carrascomj opened this issue Jul 5, 2024 · 6 comments
Open
Labels
bug Something isn't working

Comments

@carrascomj
Copy link

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
Copy link
Contributor

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

@bob-carpenter bob-carpenter added the bug Something isn't working label Jul 5, 2024
@WardBrian
Copy link
Member

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

@carrascomj
Copy link
Author

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 biosustain/Maud#469 (comment)) 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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants