Setting up PR. more unit tests would be good.

thanks for reviewing this so quickly. great feedback - will address over the weekend.

regarding CmdStan args vs. method-specific args, the CmdStan CLI requires that args be grouped under headings "method", "data", "output". Given this, you're correct that num_chains should be part of the sampler args.

Codecov Report

Merging #492 (79c1df6) into develop (725ad89) will increase coverage by 0.44%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop     #492      +/-   ##
===========================================
+ Coverage    77.95%   78.40%   +0.44%     
===========================================
  Files           30       30              
  Lines         8991     9186     +195     
===========================================
+ Hits          7009     7202     +193     
- Misses        1982     1984       +2     

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 725ad89...79c1df6. Read the comment docs.

if all unit tests pass, this is ready for re-review.

Great, I'll review after that change.

One thing that immediately strikes out to me: I think the info_dict method may be useful for other stuff in the future and might belong as part of the CmdStanModel class. That would also allow us to move the if exe_file is not None logic into the method itself, so we could just call model.info() and get back either the dictionary or None

Actually, to add on to the above comment, we should make info_dict return {} instead of None. That way we can (always) safely do something like model.info_dict().get('STAN_THREADS') without checking the return first.

the code is ready for review, but we still need to update the doc and and examples for 2.28.

the changes made on the CmdStan side w/r/t fixed_params should make people's lives easier (even if the code ties itself in knots trying to check the console outputs and CSV files).

this totally sucks - discovered that model method compile with option force=True doesn't do the right thing and there are no unit tests for this either. wtf?

making this work is important in order to be able to explain how to take advantage of new num_threads. the problem seems to be that make won't update target unless it needs to. changed logic in compile method to remove the offending exe file, but then some unit tests are failing. perhaps because of implicit dependencies? investigating.

Unless you believe that bug was introduced by this PR I think it’s better to merge this and handle that separately. These big PRs get very hard to review

this bug has been there all along - that's why unit tests are removing exe files here and there. I'm just surprised that this wasn't tested more back when we added this feature.