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

refactor _repr_latex functionality #4065

Merged
merged 14 commits into from
Sep 1, 2020

Conversation

Spaak
Copy link
Member

@Spaak Spaak commented Aug 24, 2020

This PR is the first step towards better overall string representations of PyMC3 objects, see #3956.

Before this PR, many distributions and other objects define their own _repr_latex functionality (generation of LaTeX-based string renderings for display in e.g. Jupyter notebooks). This functionality is almost entirely shared among all distributions, so this PR refactors it into one generic function Distribution._str_repr. The representation is based on the name of the distribution (_distr_name_for_repr()), as well as a list of parameters relevant for the distribution (_distr_parameters_for_repr(), e.g. mu and sigma for normal). These can be overridden by subclasses (and are in several cases, see code), but default values are the distribution's class name and the list of parameters in __init__.

Note that this PR already has a small amount of code for plain (as opposed to LaTeX) string representations in _str_repr; this is for future use, as described in #3956. This is not yet exposed (but the plan is to expose it later via the standard __str__).

@codecov
Copy link

codecov bot commented Aug 24, 2020

Codecov Report

Merging #4065 into master will increase coverage by 1.64%.
The diff coverage is 64.49%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4065      +/-   ##
==========================================
+ Coverage   86.83%   88.47%   +1.64%     
==========================================
  Files          88       88              
  Lines       14165    13860     -305     
==========================================
- Hits        12300    12263      -37     
+ Misses       1865     1597     -268     
Impacted Files Coverage Δ
pymc3/distributions/discrete.py 96.98% <50.00%> (+22.32%) ⬆️
pymc3/distributions/mixture.py 88.47% <50.00%> (+1.99%) ⬆️
pymc3/distributions/timeseries.py 85.80% <50.00%> (+13.86%) ⬆️
pymc3/distributions/continuous.py 92.78% <55.88%> (+12.68%) ⬆️
pymc3/distributions/distribution.py 92.80% <66.66%> (-1.82%) ⬇️
pymc3/distributions/multivariate.py 80.78% <70.00%> (+2.60%) ⬆️
pymc3/model.py 87.93% <71.05%> (-1.03%) ⬇️
pymc3/distributions/simulator.py 85.48% <75.00%> (-1.19%) ⬇️
pymc3/distributions/transforms.py 97.46% <75.00%> (-0.34%) ⬇️
pymc3/util.py 91.66% <75.00%> (-2.09%) ⬇️

@Spaak
Copy link
Member Author

Spaak commented Aug 27, 2020

@twiecki @ColCarroll I think this is ready for review.

(Note the low diff test coverage is due to the many _distr_params_for_repr implementations in distributions for which the LaTeX representation is not explicitly tested (as before), as well as some of the (unexposed) plain string representation code in here, for which I'll add unit tests after we tackle __str__ in general in a later PR.)

@ColCarroll
Copy link
Member

Apologies for missing this. It seems like an elegant way of making this maintainable, and I really like it.

My only suggestion is to avoid default arguments and **kwargs in private methods (_repr_str and _repr_latex specifically). I didn't look closely enough to see if this would be a major pain, but if the signature of both was something like def _repr_str(self, *, name, dist, kind):, then static type checkers would be able to do a much better job on it.

That's not a big deal, though -- feel free to just say "that'd be too hard" rather than needing to explain why! I can take a second look and merge tonight in any case.

@ColCarroll ColCarroll merged commit 96b2441 into pymc-devs:master Sep 1, 2020
@Spaak Spaak deleted the refactor-repr-latex branch September 2, 2020 12:26
@Spaak
Copy link
Member Author

Spaak commented Sep 2, 2020

My only suggestion is to avoid default arguments and **kwargs in private methods (_repr_str and _repr_latex specifically). I didn't look closely enough to see if this would be a major pain, but if the signature of both was something like def _repr_str(self, *, name, dist, kind):, then static type checkers would be able to do a much better job on it.

I had not thought of that; in general, I think my style is basically "always use **kwargs and defaults whenever possible". But I think the static type checking is a good argument, definitely against using **kwargs throughout. I'll see whether I can change this easily in a next PR.

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.

2 participants