Skip to content

Project/release 1.0 54 metric dict#453

Merged
mitzimorris merged 11 commits intodevelopfrom
project/release-1.0-54-metric-dict
Sep 9, 2021
Merged

Project/release 1.0 54 metric dict#453
mitzimorris merged 11 commits intodevelopfrom
project/release-1.0-54-metric-dict

Conversation

@mitzimorris
Copy link
Copy Markdown
Member

@mitzimorris mitzimorris commented Sep 6, 2021

Submission Checklist

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

Summary

Adds logic to allow users to specify custom metric (inv diagnoal or dense covariance metric of the unconstrained parameters) as either a Python dict containing entry 'inv_metric" or list of per-chain dicts, with corresponding unit tests.

Also did the following general code cleanups:

  • ran files `cmdstan_arg.py' and 'model.py' through spell-checker to catch typos in both docstrings and error messages.
  • corrected error messages in file cmdstan_args.py so that if error message is a proper sentence, first word is capitalized and ends with a period.

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): Columbia University

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

@mitzimorris
Copy link
Copy Markdown
Member Author

mitzimorris commented Sep 6, 2021

@WardBrian - when you get a chance - mypy isn't happy. not sure why. need your help here.
nevermind - fixed on my own. learned a bit more about Python and mypy - mypy did help make the code cleaner. allowing so many different options for arg 'metric' is a little crazy.

Copy link
Copy Markdown
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.

Added some comments

Comment thread cmdstanpy/cmdstan_args.py Outdated
Comment thread cmdstanpy/cmdstan_args.py Outdated
Comment thread cmdstanpy/cmdstan_args.py Outdated
Comment thread cmdstanpy/cmdstan_args.py
Comment thread cmdstanpy/cmdstan_args.py Outdated
Comment thread cmdstanpy/cmdstan_args.py Outdated
Comment thread cmdstanpy/cmdstan_args.py Outdated
Comment thread cmdstanpy/cmdstan_args.py Outdated
Comment thread cmdstanpy/cmdstan_args.py Outdated
Comment thread test/test_cmdstan_args.py Outdated
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Sep 6, 2021

Codecov Report

Merging #453 (5b9c1d9) into develop (3224542) will increase coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #453      +/-   ##
===========================================
+ Coverage    76.36%   76.42%   +0.05%     
===========================================
  Files           27       27              
  Lines         8979     9114     +135     
===========================================
+ Hits          6857     6965     +108     
- Misses        2122     2149      +27     
Impacted Files Coverage Δ
cmdstanpy/cmdstanpy/cmdstan_args.py 94.71% <0.00%> (-0.53%) ⬇️
...work/cmdstanpy/cmdstanpy/cmdstanpy/cmdstan_args.py 94.71% <0.00%> (-0.53%) ⬇️
a/cmdstanpy/cmdstanpy/cmdstanpy/cmdstan_args.py 93.90% <0.00%> (-0.47%) ⬇️
cmdstanpy/cmdstanpy/model.py 78.38% <0.00%> (ø)
a/cmdstanpy/cmdstanpy/cmdstanpy/model.py 80.72% <0.00%> (ø)
...runner/work/cmdstanpy/cmdstanpy/cmdstanpy/model.py 78.38% <0.00%> (ø)
cmdstanpy/cmdstanpy/__init__.py 86.95% <0.00%> (+0.59%) ⬆️
a/cmdstanpy/cmdstanpy/cmdstanpy/__init__.py 86.95% <0.00%> (+0.59%) ⬆️
...ner/work/cmdstanpy/cmdstanpy/cmdstanpy/__init__.py 86.95% <0.00%> (+0.59%) ⬆️
cmdstanpy/cmdstanpy/install_cmdstan.py 31.96% <0.00%> (+1.96%) ⬆️
... and 2 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 3224542...5b9c1d9. Read the comment docs.

Copy link
Copy Markdown
Member

@WardBrian WardBrian left a comment

Choose a reason for hiding this comment

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

Code looks good to me, one minor comment on error messaging

Comment thread cmdstanpy/cmdstan_args.py Outdated
@mitzimorris mitzimorris merged commit fb607ff into develop Sep 9, 2021
@mitzimorris mitzimorris deleted the project/release-1.0-54-metric-dict branch September 17, 2021 19:15
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