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

Don't enforce fixed_param sampler when model has no parameters #1054

Closed
wants to merge 2 commits into from

Conversation

nhuurre
Copy link

@nhuurre nhuurre commented Oct 27, 2021

Submisison Checklist

  • Run tests: ./runCmdStanTests.py src/test
  • Declare copyright holder and open-source license: see below

Summary:

Before CmdStan 2.27 you could not sample from a parameterless model with the default settings. CmdStan 2.27 fixes this annoyance by automatically switching to fixed_param sampler when model has zero parameters. (It changes not only defaults but also forces fixed_param even if HMC is explicitly specified.)
This fix however causes problems for interfaces like CmdStanPy because the interface thinks the sampler is HMC and the output CSV from fixed_param is missing expected adaptation info. PR #1030 intends to fix that issue by adjusting the sampler config recorded in the CSV file.

This PR is an alternative to #1030. Instead of modifying the config info this simply reverses the fixed_param hack; HMC is always the default. Parameterless models will be able to run HMC just fine after stan-dev/stan#3071 is merged.

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): Niko Huurre

By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses:

@mitzimorris
Copy link
Member

thank you for undoing a really ugly hack.

after stan-dev/stan#3071 plus this,
if interface user doesn't specify "fixed_param=True" and CmdStan runs HMC. what will the Stan CSV file contain?

  • will the comment header report "adaptation=False"?
  • will the output rows (following CSV row header) contain comment "# Adaptation terminated" ?

@nhuurre
Copy link
Author

nhuurre commented Oct 27, 2021

will the comment header report "adaptation=False"?

It says adapt engaged=1 so it still reports technically incorrect config. However, adaptation is irrelevant for zero-dimensional models so only possible problem would be the output format, and that brings us to...

will the output rows (following CSV row header) contain comment "# Adaptation terminated" ?

Yes. Adaptive and non-adaptive HMC samplers both use stan::services::util::mcmc_writer. They both print the final step size and metric at the end of warmup.
CmdStanPy can read the CSV file, although it mistakenly believes that the model has one unconstrained parameter and trying to read the metric fails with ValueError: could not convert string to float: ''
I haven't tested with CmdStanR.

@mitzimorris
Copy link
Member

mitzimorris commented Oct 27, 2021

CmdStanPy can read the CSV file, although it mistakenly believes that the model has one unconstrained parameter and trying to read the metric fails with ValueError: could not convert string to float: ''

thanks for checking. the problem is some combination of logic here:

will fix for next release.

@WardBrian
Copy link
Member

Is this a safe bet for 2.29? Downstream processing would be a lot easier

@nhuurre
Copy link
Author

nhuurre commented Nov 24, 2021

Is this a safe bet for 2.29?

This is blocked on #1057. If that's going to be resolved with just a better error message then this PR isn't viable; the adaptive and nonadaptive HMC samplers must be interchangeable.

@nhuurre
Copy link
Author

nhuurre commented Nov 29, 2021

Disregard the last comment; this works even without #1057 because stan-dev/stan#3071 now fixes adaptive samplers too.

@mitzimorris
Copy link
Member

is this outdated? close PR?

@nhuurre nhuurre deleted the hmc-with-no-parameters branch August 6, 2024 06:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants