Opened 3 years ago

Closed 3 years ago

#25799 closed defect (fixed)

Remove hardcoded default matplotlib stylesheet

Reported by: klee Owned by:
Priority: major Milestone: sage-8.4
Component: graphics Keywords:
Cc: fbissey, strogdon Merged in:
Authors: Kwankyu Lee Reviewers: Eric Gourgoulhon, François Bissey
Report Upstream: N/A Work issues:
Branch: 42bda6f (Commits, GitHub, GitLab) Commit: 42bda6f486e45c89daf5229ce89034dd5f0da0fb
Dependencies: Stopgaps:

Status badges

Description (last modified by klee)

This does not work:

import matplotlib as mpl 
mpl.rcParams['font.family'] = 'Impact'
text("A text here", (1,1))

because of hardcoded matplotlib stylesheet (classic), which silently overwrites the rc parameters. Putting configurations in the file matplotlibrc also does not work because of the same reason.

The patch removes the hardcoded default matplotlib stylesheet that silently applies to all graphics. With this patch, the usual method to apply a style to graphics works, like

import matplotlib.pyplot as plt
plt.style.use('seaborn')
text("A text here", (1,1))

There was more discussion on this matter before at ticket:23696#comment:111.

Attachments (6)

plot.png (38.3 KB) - added by egourgoulhon 3 years ago.
plot_classic.png (34.5 KB) - added by egourgoulhon 3 years ago.
dashed_classic.png (30.3 KB) - added by klee 3 years ago.
dashed_default.png (27.0 KB) - added by klee 3 years ago.
dashed2_classic.png (24.3 KB) - added by klee 3 years ago.
dashed2_default.png (21.6 KB) - added by klee 3 years ago.

Download all attachments as: .zip

Change History (37)

comment:1 Changed 3 years ago by klee

  • Branch set to u/klee/25799

comment:2 Changed 3 years ago by klee

  • Description modified (diff)

comment:3 Changed 3 years ago by git

  • Commit set to 91736728f417dd7598a0eff1ec30bf6125a6fa2a

Branch pushed to git repo; I updated commit sha1. New commits:

9173672Remove hardcoded matplotlib stylesheet

comment:4 Changed 3 years ago by klee

  • Status changed from new to needs_review

comment:5 Changed 3 years ago by klee

  • Description modified (diff)

comment:6 Changed 3 years ago by egourgoulhon

For reference, I am copying below a part of this sage-devel thread:

we may keep specifically only the math font of the classic style by:

mpl.rcParams['mathtext.fontset'] = 'cm'
mpl.rcParams['mathtext.rm'] = 'serif'

See this example for drawbacks of using Matplolib new defaults for math fonts. Moreover using cm fonts for math makes plots coherent with MathJax rendering of Jupyter output cells, which is an important feature IMHO.

comment:7 Changed 3 years ago by git

  • Commit changed from 91736728f417dd7598a0eff1ec30bf6125a6fa2a to 7828ce2440ed53505c8c6e10e6a16effe1c2e4b5

Branch pushed to git repo; I updated commit sha1. New commits:

7828ce2Set computer modern fonts for math text in graphics

comment:8 Changed 3 years ago by klee

  • Description modified (diff)

comment:9 Changed 3 years ago by egourgoulhon

  • Priority changed from minor to major

Changing the priority to 'major' in order to get the attention of patchbots.

Changed 3 years ago by egourgoulhon

Changed 3 years ago by egourgoulhon

comment:10 follow-up: Changed 3 years ago by egourgoulhon

A noticeable effect of the proposed change is that it decreases the size of figures of approx. 20% in each dimension. For instance, with the ticket branch,

sage: plot(sin(x^2), (x, 0, 4), axes_labels=['$x$', '$y$'], gridlines=True)

generates this 628x470 figure:

while we had previously (i.e. with the 'classic' style) this 781x584 figure:

This size change was already discussed in comment:134 of #23696.

Other changes are the major ticks on the x-axis (Delta_x = 0.5 instead of Delta_x = 1) and the dash style (in general, dashes are narrower than with the classic style, as we can see on the dash plots in the 2D graphics section of the documentation).

I am fine with these changes (*); I simply mention them for the benefit of the discussion.

(*) except maybe for the dash style of curves, which could require some tuning of the option linestyle='--' on Sage's side: compare the output of

sage: g1 = plot(sin(x), 0, 2*pi)
sage: g2 = plot(cos(x), 0, 2*pi, linestyle="--")
sage: (g1+g2).show(ticks=pi/6, tick_formatter=pi)  # long time # show their sum, nicely formatted

in the 2D plotting section of the documentation.

On general grounds, I think it is a good thing to remove the 'classic' stylesheet (provided the TeX fonts are corrected as above), in particular because this fixes the issues mentioned in the ticket description.

comment:11 Changed 3 years ago by egourgoulhon

  • Cc fbissey strogdon added

Changed 3 years ago by klee

Changed 3 years ago by klee

comment:12 in reply to: ↑ 10 Changed 3 years ago by klee

Replying to egourgoulhon:

I am fine with these changes (*); I simply mention them for the benefit of the discussion.

(*) except maybe for the dash style of curves, which could require some tuning of the option linestyle='--' on Sage's side: compare the output of

sage: g1 = plot(sin(x), 0, 2*pi)
sage: g2 = plot(cos(x), 0, 2*pi, linestyle="--")
sage: (g1+g2).show(ticks=pi/6, tick_formatter=pi)  # long time # show their sum, nicely formatted

in the 2D plotting section of the documentation.

I am not sure...

Dashed line in classic style:

Dashed line in matplotlib2 default style:

comment:13 follow-up: Changed 3 years ago by fbissey

While the font change was the most glaring thing, there was other stuff indeed. Changing the style was the shortest way to fix all that. It would be interesting to know if we can set a style just for building the documentation. Of course even that could cause problems if you need to change something for building the documentation in a specific language.

comment:14 in reply to: ↑ 13 Changed 3 years ago by klee

Replying to fbissey:

While the font change was the most glaring thing, there was other stuff indeed. Changing the style was the shortest way to fix all that.

If there are needed other (not uniformly agreed upon) tunings, then it may be necessary to make another ticket for that purpose, while the most glaring changes, like the font change, get into this ticket. Let's see.

It would be interesting to know if we can set a style just for building the documentation. Of course even that could cause problems if you need to change something for building the documentation in a specific language.

Yes. But as someone has suggested in other context, I agree that users should get the same outputs in their own sessions as in the documentation.

Changed 3 years ago by klee

Changed 3 years ago by klee

comment:15 Changed 3 years ago by klee

Just for discussion.

Dashed linesin classic style:

Dashed lines in default style:

Version 0, edited 3 years ago by klee (next)

comment:16 Changed 3 years ago by klee

There was a poll about the changes that the branch of this ticket introduces in

https://groups.google.com/forum/#!topic/sage-devel/jz9fCnW8eT8

Counted from 8 participants to the discussion, there are

5 positive votes;

2 no opinion;

1 not-so-positive vote.

comment:17 Changed 3 years ago by egourgoulhon

  • Milestone changed from sage-8.3 to sage-8.4
  • Reviewers set to Eric Gourgoulhon
  • Status changed from needs_review to positive_review

So let's move this into Sage 8.4!

comment:18 Changed 3 years ago by egourgoulhon

  • Authors set to Kwankyu Lee

comment:19 follow-up: Changed 3 years ago by vbraun

  • Status changed from positive_review to needs_work
sage -t --long src/sage/plot/arrow.py  # 1 doctest failed

comment:20 in reply to: ↑ 19 ; follow-up: Changed 3 years ago by fbissey

Replying to vbraun:

sage -t --long src/sage/plot/arrow.py  # 1 doctest failed

That doctest is fragile and I am pretty sure it depends on the stylesheet used.

comment:21 in reply to: ↑ 20 Changed 3 years ago by egourgoulhon

Replying to fbissey:

Replying to vbraun:

sage -t --long src/sage/plot/arrow.py  # 1 doctest failed

That doctest is fragile and I am pretty sure it depends on the stylesheet used.

I confirm the doctest fails with the ticket branch merged in Sage 8.3.rc0:

./sage -t --long src/sage/plot/arrow.py 
Running doctests with ID 2018-07-30-23-45-27-5227bad0.
Git branch: matplotlib_style
Using --optional=mpir,python2,sage
Doctesting 1 file.
sage -t --long --warn-long 80.1 src/sage/plot/arrow.py
**********************************************************************
File "src/sage/plot/arrow.py", line 364, in sage.plot.arrow.Arrow._render_on_subplot
Failed example:
    two_stroke_re.search(contents) is None
Expected:
    True
Got:
    False
**********************************************************************
1 item had failures:
   1 of  18 in sage.plot.arrow.Arrow._render_on_subplot
    [69 tests, 1 failure, 3.79 s]

comment:22 follow-up: Changed 3 years ago by fbissey

Getting that doctest to return something sensible required a considerable amount of effort. It is about controlling some aspect of the appearance on screen and stems from an older ticket about the appearance of double dashing. If I am completely honest I am not sure it is worth the time invested into fixing it.

comment:23 in reply to: ↑ 22 ; follow-up: Changed 3 years ago by klee

Replying to fbissey:

Getting that doctest to return something sensible required a considerable amount of effort. It is about controlling some aspect of the appearance on screen and stems from an older ticket about the appearance of double dashing. If I am completely honest I am not sure it is worth the time invested into fixing it.

I agree. Doctesting graphics is something that requires a human eye or AI.

comment:24 Changed 3 years ago by git

  • Commit changed from 7828ce2440ed53505c8c6e10e6a16effe1c2e4b5 to 42bda6f486e45c89daf5229ce89034dd5f0da0fb

Branch pushed to git repo; I updated commit sha1. New commits:

a825afbRemove a fragile failing doctest
42bda6fAdd a comment on the removed doctest

comment:25 Changed 3 years ago by klee

  • Status changed from needs_work to needs_review

comment:26 Changed 3 years ago by egourgoulhon

  • Reviewers changed from Eric Gourgoulhon to Eric Gourgoulhon, François Bissey
  • Status changed from needs_review to positive_review

LGTM. François, do you agree?

comment:27 Changed 3 years ago by fbissey

I am OK with it. I'll be waiting for comments on the new look of the documentation. It is inevitable, even when we have talked so much about it.

comment:28 in reply to: ↑ 23 ; follow-up: Changed 3 years ago by strogdon

Replying to klee:

Replying to fbissey:

Getting that doctest to return something sensible required a considerable amount of effort. It is about controlling some aspect of the appearance on screen and stems from an older ticket about the appearance of double dashing. If I am completely honest I am not sure it is worth the time invested into fixing it.

I agree. Doctesting graphics is something that requires a human eye or AI.

It appears that now we are back to the matching sequence for 1.5.3 https://trac.sagemath.org/ticket/23696#comment:27. This

sage: two_stroke_pattern = r'setdash.*setdash.*stroke.*stroke.*setdash'

seems to work; although, I haven't checked whether the above is matched correctly when the arrow is drawn with dashed linetype.

comment:29 in reply to: ↑ 28 Changed 3 years ago by strogdon

Replying to strogdon:

Replying to klee:

Replying to fbissey:

Getting that doctest to return something sensible required a considerable amount of effort. It is about controlling some aspect of the appearance on screen and stems from an older ticket about the appearance of double dashing. If I am completely honest I am not sure it is worth the time invested into fixing it.

I agree. Doctesting graphics is something that requires a human eye or AI.

It appears that now we are back to the matching sequence for 1.5.3 https://trac.sagemath.org/ticket/23696#comment:27. This

sage: two_stroke_pattern = r'setdash.*setdash.*stroke.*stroke.*setdash'

seems to work; although, I haven't checked whether the above is matched correctly when the arrow is drawn with dashed linetype.

This was just a shot in the dark. It doesn't work properly when the arrow is drawn with dashed linetype.

comment:30 Changed 3 years ago by strogdon

Just for the record, not that anything should be changed but

sage: two_stroke_pattern = r'setdash.*setdash.*stroke.*stroke.*setdash'

is the correct matching pattern. It's fairly straightforward to deduce it. I had a typo that led to the previous conclusion.

comment:31 Changed 3 years ago by vbraun

  • Branch changed from u/klee/25799 to 42bda6f486e45c89daf5229ce89034dd5f0da0fb
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.