Skip to content

ENH: Add support for ipython latex printing to polynomial#11528

Merged
charris merged 1 commit intonumpy:masterfrom
eric-wieser:polynomial-ipython-latex
Aug 12, 2018
Merged

ENH: Add support for ipython latex printing to polynomial#11528
charris merged 1 commit intonumpy:masterfrom
eric-wieser:polynomial-ipython-latex

Conversation

@eric-wieser
Copy link
Copy Markdown
Member

@eric-wieser eric-wieser commented Jul 8, 2018

Choices made, and the alternatives rejected (for no particularly strong reason):

  1. Show terms in ascending order, to match their internal representation
    • alternative: descending, to match convention
  2. Shows 0 terms in gray
    • alternative: omit entirely
    • alternative: show normally to aid comparison
  3. Write each term as `basis(ax + b)
    • alternative: write as basis(u) ... where u = ax + b
    • alternative: show the normalized polynomial

In future it would perhaps make sense to expose these options to the end user

Copy link
Copy Markdown
Member Author

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

Shown with some mathjax renderings

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

image

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

image

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

image

@charris
Copy link
Copy Markdown
Member

charris commented Jul 9, 2018

I think we may want to revisit 3., especially as the terms extend to higher powers and the mapping ax + b has full precision coefficients, which is likely to happen when doing fits to data scattered along the x axis. Using u = a x + b explicitly is also how one would want to evaluate the polynomial or publish it. We will probably want to add some options for the order also. I think the domain is also needed, especially for fitting, where it is the domain in which the polynomial is valid. For high order fits the polynomial will try to run off to inf almost immediately outside the domain.

That said, this PR looks like a good starting point that will generate useful feedback.

@eric-wieser
Copy link
Copy Markdown
Member Author

I think we may want to revisit 3

Yes, I agree - that was the choice I was least sure of. Shall I do 0, 1, or both of the following?

  1. Change it to \text{ where } u = ...
  2. Add a np.polynomial.set_latex_printoptions(substitute=False) to tweak this at runtime

that will generate useful feedback

Do you think we should gain most of the feedback before or after merging?

It's not clear to me how best to run jupyter notebook with runtests.py.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Unfortunately '{}'.format(some_float) doesn't generate valid latex for coefficients using scientific notation. I'm not sure what the best thing to do here is - do we show the coefficients as monospaced and keep the e, to make it easy to copy/paste them?

Copy link
Copy Markdown
Member

@charris charris Jul 9, 2018

Choose a reason for hiding this comment

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

do we show the coefficients as monospaced and keep the e, to make it easy to copy/paste them?

Makes sense, at least in the near term. This might be one of the things that garners feedback. We should probably decide how to set various options for the display, but that can wait.

Do you think we should gain most of the feedback before or after merging?

I was thinking after merge. I don't think it matters much if we change things later, and we won't know what people want in practice until we have people using it :) Maybe mark it as experimental in the release notes. Can make it a highlight just so folks see it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, this part of latex formatting is annoying; we have a specific method that does it for units/quantities

@mattip
Copy link
Copy Markdown
Member

mattip commented Jul 9, 2018

For those following along and wondering what this does, ipython front-ends can call "rich display" methods to display a class object.

@mattip mattip added the 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes label Jul 9, 2018
@ev-br
Copy link
Copy Markdown
Contributor

ev-br commented Jul 15, 2018

A big +1 for this from the peanut gallery.

@eric-wieser eric-wieser self-assigned this Jul 23, 2018
@charris
Copy link
Copy Markdown
Member

charris commented Aug 12, 2018

Needs a release note.

@eric-wieser eric-wieser force-pushed the polynomial-ipython-latex branch from 36ca790 to 1457b53 Compare August 12, 2018 18:56
Choices made, and the alternatives rejected (for no particularly strong reason):

1. Show terms in ascending order, to match their internal representation
  * alternative: descending, to match convention
2. Shows 0 terms in gray
  * alternative: omit entirely
  * alternative: show normally to aid comparison
3. Write each term as `basis(ax + b)
  * alternative: write as `basis(u) ... where u = ax + b`
  * alternative: show the normalized polynomial

In future it would perhaps make sense to expose these options to the end user
@eric-wieser eric-wieser force-pushed the polynomial-ipython-latex branch from 1457b53 to e6e60c0 Compare August 12, 2018 21:34
@codecov-io
Copy link
Copy Markdown

codecov-io commented Aug 12, 2018

Codecov Report

Merging #11528 into master will decrease coverage by 0.63%.
The diff coverage is 91.46%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11528      +/-   ##
==========================================
- Coverage   86.58%   85.95%   -0.64%     
==========================================
  Files         315      326      +11     
  Lines       81307    81990     +683     
==========================================
+ Hits        70401    70474      +73     
- Misses      10906    11516     +610
Impacted Files Coverage Δ
numpy/polynomial/chebyshev.py 92.98% <100%> (+0.01%) ⬆️
numpy/polynomial/legendre.py 95.67% <100%> (+0.01%) ⬆️
numpy/polynomial/hermite_e.py 95.46% <100%> (+0.01%) ⬆️
numpy/polynomial/hermite.py 95.49% <100%> (+0.01%) ⬆️
numpy/polynomial/polynomial.py 95.46% <100%> (+0.11%) ⬆️
numpy/polynomial/laguerre.py 95.58% <100%> (+0.01%) ⬆️
numpy/polynomial/tests/test_classes.py 99.34% <100%> (+0.03%) ⬆️
numpy/polynomial/_polybase.py 78.75% <84.44%> (+0.68%) ⬆️
numpy/lib/setup.py 0% <0%> (ø)
numpy/compat/setup.py 0% <0%> (ø)
... and 9 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 b007246...e6e60c0. Read the comment docs.

@charris charris merged commit a524915 into numpy:master Aug 12, 2018
@charris
Copy link
Copy Markdown
Member

charris commented Aug 12, 2018

Seems users can toggle pretty printing with %pprint, which will hopefully disable this if big polynomials get out of hand. I think the only way we will get useful feedback is to put it out there and see what happens. If it goes wrong there is always the 1.16.1 release :) Thanks Eric.

@eric-wieser
Copy link
Copy Markdown
Member Author

The sad reality might be that almost no one uses np.polynomial anyway, and people are stuck on np.poly1d and friends...

@mhvk
Copy link
Copy Markdown
Contributor

mhvk commented Aug 13, 2018

I do use it...

eric-wieser pushed a commit that referenced this pull request Sep 4, 2018
This variable is never used, and shows signs of being leftover from a previous implementation.

Introduced in gh-11528. Closes gh-11842
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants