Opened 4 years ago
Closed 3 years ago
#25799 closed defect (fixed)
Remove hardcoded default matplotlib stylesheet
Reported by:  klee  Owned by:  

Priority:  major  Milestone:  sage8.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: 
Description (last modified by )
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)
Change History (37)
comment:1 Changed 4 years ago by
 Branch set to u/klee/25799
comment:2 Changed 4 years ago by
 Description modified (diff)
comment:3 Changed 4 years ago by
 Commit set to 91736728f417dd7598a0eff1ec30bf6125a6fa2a
comment:4 Changed 4 years ago by
 Status changed from new to needs_review
comment:5 Changed 4 years ago by
 Description modified (diff)
comment:6 Changed 4 years ago by
For reference, I am copying below a part of this sagedevel 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 4 years ago by
 Commit changed from 91736728f417dd7598a0eff1ec30bf6125a6fa2a to 7828ce2440ed53505c8c6e10e6a16effe1c2e4b5
Branch pushed to git repo; I updated commit sha1. New commits:
7828ce2  Set computer modern fonts for math text in graphics

comment:8 Changed 4 years ago by
 Description modified (diff)
comment:9 Changed 4 years ago by
 Priority changed from minor to major
Changing the priority to 'major' in order to get the attention of patchbots.
Changed 4 years ago by
Changed 4 years ago by
comment:10 followup: ↓ 12 Changed 4 years ago by
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 xaxis (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 4 years ago by
 Cc fbissey strogdon added
Changed 4 years ago by
Changed 4 years ago by
comment:12 in reply to: ↑ 10 Changed 4 years ago by
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 ofsage: 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 formattedin the 2D plotting section of the documentation.
I am not sure...
Dashed line in classic style:
Dashed line in matplotlib2 default style:
comment:13 followup: ↓ 14 Changed 4 years ago by
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 4 years ago by
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 4 years ago by
Changed 4 years ago by
comment:15 Changed 4 years ago by
comment:16 Changed 4 years ago by
There was a poll about the changes that the branch of this ticket introduces in
https://groups.google.com/forum/#!topic/sagedevel/jz9fCnW8eT8
Counted from 8 participants to the discussion, there are
5 positive votes;
2 no opinion;
1 notsopositive vote.
comment:17 Changed 4 years ago by
 Milestone changed from sage8.3 to sage8.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 4 years ago by
comment:19 followup: ↓ 20 Changed 4 years ago by
 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 ; followup: ↓ 21 Changed 3 years ago by
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
Replying to fbissey:
Replying to vbraun:
sage t long src/sage/plot/arrow.py # 1 doctest failedThat 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 201807302345275227bad0. Git branch: matplotlib_style Using optional=mpir,python2,sage Doctesting 1 file. sage t long warnlong 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 followup: ↓ 23 Changed 3 years ago by
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 ; followup: ↓ 28 Changed 3 years ago by
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
 Commit changed from 7828ce2440ed53505c8c6e10e6a16effe1c2e4b5 to 42bda6f486e45c89daf5229ce89034dd5f0da0fb
comment:25 Changed 3 years ago by
 Status changed from needs_work to needs_review
comment:26 Changed 3 years ago by
 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
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 ; followup: ↓ 29 Changed 3 years ago by
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
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 for1.5.3
https://trac.sagemath.org/ticket/23696#comment:27. Thissage: 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 withdashed
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
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
 Branch changed from u/klee/25799 to 42bda6f486e45c89daf5229ce89034dd5f0da0fb
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
Remove hardcoded matplotlib stylesheet