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

Add str/repr formatting options and change defaults accordingly #4260

Merged
merged 4 commits into from
Nov 27, 2020

Conversation

michaelosthege
Copy link
Member

@michaelosthege michaelosthege commented Nov 26, 2020

  • new options "latex_with_params" and "plain_with_params" replace the current default behavior of including input parameters (going back to default behaviour of the 3.9.3 release)
  • __latex__ and _repr_latex default to "latex_with_params"
  • __str__ and _str_repr default to "plain"
  • __latex__ repr of a model defaults to "latex" (without params)
  • new formatting kwarg for model_to_graphviz enables switching between "plain" (default) and "plain_with_params"

Depending on what your PR does, here are a few things you might want to address in the description:

  • what are the (breaking) changes that this PR makes? - None
  • important background, or details about the implementation
  • are the changes—especially new features—covered by tests and docstrings?
  • consider adding/updating relevant example notebooks
  • right before it's ready to merge, mention the PR in the RELEASE-NOTES.md

+ new options "latex_with_params" and "plain_with_params" replace the current default behavior of including input parameters (going back to default behaviour of the 3.9.3 release)
+ __latex__ and _repr_latex default to "latex_with_params"
+ __str__ and _str_repr default to "plain"
+ new formatting kwarg for model_to_graphviz enables switching between "plain" (default) and "plain_with_params"
+ latex formatting should be detected by `if "latex" in formatting` to catch both format options
+ all latex reprs except for an entire model default to "latex_with_params"
+ tests now cover cases with and without params
+ test_issue_4186 was merged into TestStrAndLatexRepr and now all 4 formatting options are covered.
@Spaak
Copy link
Member

Spaak commented Nov 26, 2020

Thanks @michaelosthege!

I'm wondering what should be the default option. Right now the long "f(f(f(...)))" strings are definitely undesirable as defaults, I agree. (Although I think these have always been the default in the LaTeX representation in notebooks, by the way.) If we simplify these to always be compact, I'd say the default should be with the params. (Especially in console sessions, simply typing myvar it's nice to get myvar ~ Normal(mu=0.0, sigma=1.0) without having to explicitly specify a format.)

@michaelosthege
Copy link
Member Author

Thanks @michaelosthege!

I'm wondering what should be the default option. Right now the long "f(f(f(...)))" strings are definitely undesirable as defaults, I agree. (Although I think these have always been the default in the LaTeX representation in notebooks, by the way.) If we simplify these to always be compact, I'd say the default should be with the params. (Especially in console sessions, simply typing myvar it's nice to get myvar ~ Normal(mu=0.0, sigma=1.0) without having to explicitly specify a format.)

For simple RVs yes, but in most real-world models, these representations will be huge expressions... And they also show up in tracebacks that are already hard to read.

If we adopt a global rcParams style output configuration (like ArviZ) the user could change that default.

@codecov
Copy link

codecov bot commented Nov 26, 2020

Codecov Report

Merging #4260 (15ce7bf) into master (a6295a3) will decrease coverage by 0.02%.
The diff coverage is 88.37%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4260      +/-   ##
==========================================
- Coverage   88.15%   88.13%   -0.03%     
==========================================
  Files          87       87              
  Lines       14243    14257      +14     
==========================================
+ Hits        12556    12565       +9     
- Misses       1687     1692       +5     
Impacted Files Coverage Δ
pymc3/distributions/bart.py 80.80% <0.00%> (ø)
pymc3/distributions/distribution.py 94.31% <83.33%> (-0.16%) ⬇️
pymc3/model.py 89.00% <88.88%> (-0.17%) ⬇️
pymc3/distributions/bound.py 92.36% <100.00%> (ø)
pymc3/distributions/simulator.py 82.19% <100.00%> (ø)
pymc3/model_graph.py 88.57% <100.00%> (+0.16%) ⬆️
pymc3/backends/report.py 90.90% <0.00%> (-2.10%) ⬇️
pymc3/distributions/continuous.py 93.27% <0.00%> (+0.11%) ⬆️

"""Magic method name for IPython to use for LaTeX formatting."""
return self._str_repr(formatting="latex", **kwargs)
return self._str_repr(formatting=formatting, **kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised that codecov is complaining that this line is not covered by tests, as far as I can tell it should be?

@Spaak
Copy link
Member

Spaak commented Nov 27, 2020

Looks good to me! See my one review comment about a line somehow missing tests (or malfunctioning codecov?). Also we're still waiting on Travis before we can merge...

I might file a later PR for your review @michaelosthege which improves (drastically shortens) the display of parameter values, such that maybe that can be the default (again)? For now I agree better to stick with this version.

@MarcoGorelli
Copy link
Contributor

MarcoGorelli commented Nov 27, 2020

we're still waiting on Travis before we can merge...

Tests are now run via GitHubActions, Travis can be ignored

I'll just have a quick look regarding the non-covered line

EDIT: just noticed there's a test failure on master, I'll take a look at that, no need to wait for me on this one

@MarcoGorelli MarcoGorelli requested review from MarcoGorelli and removed request for MarcoGorelli November 27, 2020 07:57
@twiecki twiecki merged commit 27b1a7d into pymc-devs:master Nov 27, 2020
@twiecki
Copy link
Member

twiecki commented Nov 27, 2020

Thanks @michaelosthege and @Spaak!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants