Skip to content

Conversation

WardBrian
Copy link
Member

Submission Checklist

  • Run unit tests
  • Declare copyright holder and open-source license: see below

Summary

Add model.format_model. This calls stanc --auto-format, with optional arguments to 'canonicalize' the program. By default the formatted model is printed, if save=True is passed it is written back out to the file on disk, after first creating a copy (named filename.stan.bak)

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):
Simons Foundation

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

@WardBrian WardBrian added the feature New feature or request label Feb 15, 2022
Copy link
Contributor

@ahartikainen ahartikainen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of comments

file.write(result)
else:
print(result)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Printing is not very pythonic. Should it just return the result?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am thinking of how a user would use this method and I have a hard time imagining when they'd actually want the string as a result. If anything it encourages the style of programming we don't want, where the model code ends up stored as a string in the python code.

The workflow I'm imagining for this function is that it will either be used so that someone can preview the output, in which case printing is the right thing to do, or they will be batch-processing stan files and formatting them, in which case they'd be saving them back to disk.

I can be persuaded that this is too narrow, however

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I think printing is ok in this case

@WardBrian
Copy link
Member Author

I want to write a few more test cases but need to update my local cmdstan to 2.29 to do so first. I'll try to do this by the end of the day

@codecov-commenter
Copy link

codecov-commenter commented Feb 15, 2022

Codecov Report

Merging #539 (26e86d5) into develop (d839f02) will decrease coverage by 0.16%.
The diff coverage is n/a.

❗ Current head 26e86d5 differs from pull request most recent head e5d7531. Consider uploading reports for the commit e5d7531 to get more accurate results

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #539      +/-   ##
===========================================
- Coverage    79.41%   79.25%   -0.17%     
===========================================
  Files           45       15      -30     
  Lines         9600     3234    -6366     
===========================================
- Hits          7624     2563    -5061     
+ Misses        1976      671    -1305     
Impacted Files Coverage Δ
a/cmdstanpy/cmdstanpy/cmdstanpy/cmdstan_args.py
a/cmdstanpy/cmdstanpy/cmdstanpy/utils.py
...runner/work/cmdstanpy/cmdstanpy/cmdstanpy/utils.py
a/cmdstanpy/cmdstanpy/cmdstanpy/_version.py
...ork/cmdstanpy/cmdstanpy/cmdstanpy/compiler_opts.py
a/cmdstanpy/cmdstanpy/cmdstanpy/progress.py
...work/cmdstanpy/cmdstanpy/cmdstanpy/cmdstan_args.py
a/cmdstanpy/cmdstanpy/cmdstanpy/model.py
a/cmdstanpy/cmdstanpy/cmdstanpy/stanfit/mle.py
...runner/work/cmdstanpy/cmdstanpy/cmdstanpy/model.py
... and 21 more

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 d839f02...e5d7531. Read the comment docs.

@rok-cesnovar
Copy link
Member

Sorry for jumping late, I am about to add the same thing for cmdstanr and was just wondering about naming. I know naming is hard and I know I am jumping in the last minutes, just bouncing a few ideas that come to mind before we release this feature.

  • model.format_model: the second model seems redundant. What about just model.format()?
  • save arg: how about overwrite_file? @spinkney suggested that. I don't have a preference, though overwrite_file does seem more self-explanatory
  • unsafe: maybe rather than unsafe that defaults to False, backup that defaults to True?

@WardBrian
Copy link
Member Author

I agree on all three counts, model.format(), overwrite_file and backup

@rok-cesnovar
Copy link
Member

rok-cesnovar commented Feb 15, 2022

One more suggestion: exposing max_line_length?

EDIT: I did add it in the CmdStanR PR.

@WardBrian
Copy link
Member Author

Will do that as well, hopefully tomorrow

@WardBrian WardBrian merged commit ef23b3b into develop Feb 16, 2022
@WardBrian WardBrian deleted the model-formatting branch February 16, 2022 17:01
@WardBrian WardBrian mentioned this pull request May 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants