-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Allow custom documentclass for pgf backend #28167
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
base: main
Are you sure you want to change the base?
Conversation
The goal is for this to work with any arbitrary |
db62b70
to
c6a80a5
Compare
Is there any reason why matplotlib inserts some LaTeX commands (e.g. for the In other words I'm suggesting to:
If there are no objections, I'll make these changes. |
db73b44
to
e2d1aa7
Compare
da9ea78
to
d3e4b41
Compare
Users should set the document font size in their documentclass instead.
All user defined LaTeX code is now in one place.
These commands are run before using the underscore package.
d3e4b41
to
447379b
Compare
This PR solves multiple issues. Please let me know if these should be split into multiple PRs:
|
The changes I made to the docs were fairly simple, so I don't understand why the build fails. If anyone knows, I would appreciate the insight. |
This PR also has a known regression. It fixes most custom preambles, but I know of at least one that it will break. The following works before this PR but breaks after: mpl.rcParams['pgf.preamble'] = r'\usepackage[strings]{underscore}' Matplotlib uses the I assume this regression is much more rare than the bug it fixes, so I think this PR should still be considered a net improvement. If this gets merged, then a new issue should probably be opened for this regression. I'm not sure how to solve this (maybe something using |
attn @pwuertz |
For better or worse, the use of I have concerns about introducing a known regression to add a feature. Given that we only import a small number packages by default, can we detect if the user also used them and either move our invocation around or drop adding it? |
To be clear, this PR both fixes a bug (#28213) and adds a new feature (#28119). The above regression is caused by the reordering of packages (specifically the Unfortunately, package ordering conflicts are a huge pain in LaTeX. Since we're taking an arbitrary user preamble, the current version of matplotlib (pre-PR) probably has plenty of them. I only mentioned This PR does essentially drop adding them if they're already loaded, but that only fixes the option clash bugs. It does not solve the problem where
SolutionsThe ideal solution would be a proper automated topological sorting of dependencies that would be transparent to us. There was an effort made to automatically resolve known conflicts, but sadly this has been abandoned. I think LaTeX3 was also supposed to help as well, but for the current LaTeX2e, we're stuck. The most practical solution I can think of is to leave the responsibility up to the user if they choose to create a preamble. In other words, if the user preamble is non-empty, then matplotlib would not inject any packages that cause conflicts and the user must handle it themselves. Alternatively, yet another rcParam flag could be used to prevent injecting them. Basically, if in the worst case we can't guarantee it won't crash, then we should document the feature as experimental, but also give the user the tools they need to put out the fire they create. |
I see, bug-for-bug is possibly a better trade Maybe the right fix here is to move the default preamble into the default rc (so at least the user can see what minimal working preamble is). For any of these changes though, how do effectively warn users during the transition period? Can we check if the preamble has been customized and if so if it contains the needed imports. If not put them in someplace and warn that we will stop doing that in the future. In the future we can either just warn critical things are missing or raise a helpful error message that has instructions of what needs to be added (and a link to an explanation of why we can not just add it our self?) |
Your solution of moving the default preamble to the default value of Below are my thoughts on some of the details for transitioning. Warnings to UserI think we can check if the preamble loaded the necessary packages and options by using I'm not quite sure the best way to warn the user. One option could be to run the user preamble without modification except for a subsequent check to see if the packages were loaded by using Before the future deprecation we would do the following:
After the future deprecation, the 2nd re-run of the LaTeX compiler should be removed and we would do the following:
Temporary MitigationThe "someplace" that is chosen will inherently sometimes fail for the reasons we previously discussed.
I would probably opt for option 2 and abandon the "bug-for-bug" mitigations in this PR. |
I also like option 2 better. I like the plan of checking if the package is loaded and forcing a crash (I did not know |
PR summary
This allows the user to set a custom
documentclass
for the LaTeX compiler to use. It is recommended to combine this withpgf.preamble
and have both match the LaTeX document that the pgf will appear in. This helps fix consistency issues with fonts, sizes, positions, etc. These bugs were typically caused by differences when rendering the pgf with matplotlib versus the final rendering of the LaTeX document. Fixes #28119 and fixes #28213.When combined with #26893, many consistency issues should be avoidable.
PR checklist