-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Refactored LaTeX generation in preview #22734
Conversation
✅ Hi, I am the SymPy bot (v167). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.
Click here to see the pull request description that was parsed.
|
24a1ed8
to
6cd24d9
Compare
Benchmark results from GitHub Actions Lower numbers are good, higher numbers are bad. A ratio less than 1 Significantly changed benchmark results (PR vs master) Significantly changed benchmark results (master vs previous release) before after ratio
[77f1d79c] [eb9caeb0]
<sympy-1.10.1^0>
+ 97.9±1ms 177±0.6ms 1.81 sum.TimeSum.time_doit
Full benchmark results can be found as artifacts in GitHub Actions |
I think this looks good although I'm not really sure how to test it. I don't think I've seen |
All the LaTeX generation (like in IPython and Spyder) goes through (I have problems testing the other aspects of preview as well.) |
Ideally |
I was thinking about adding a |
Doing NotImplementedError on Windows for now would be fine. I will say my experience on Windows in general is that you shouldn't delete files that are open in programs like you would in Unix, so you'd have to find a way to only delete the file after the program closes. Perhaps Windows has some API for this? |
Except from that the feature request from @asmeurer about an ability to open a file in the system viewer is not implemented, this PR does what it originally was supposed to do: generate the LaTeX-file in a single place and use the same LaTeX-file for different cases. I think it may just as well go into 1.11. |
Not sure what the circleci error is. I guess that's because this PR predates using circleci. @asmeurer does the circleci build use the PR branch or does it use the branch resulting from merging the PR into master (like most other things in CI do)? |
@oscargus I haven't looked through this in detail but it looks fine. Feel free to merge if you're happy with it. |
6cd24d9
to
65f47d9
Compare
029b293
to
7849b93
Compare
7849b93
to
15be357
Compare
I changed the name of |
I rebased and then the CircleCI stuff was OK (I guess this was pretty old). |
I think something funny happened with the rebase because it's showing extra commits that were already on master. |
Ahh! Sorry about that! And automerge on top of that... |
References to other Issues or PRs
Brief description of what is fixed or changed
Refactored the interactive LaTeX generation a bit so that the LaTeX preamble etc are defined in a single place.
Other comments
As an added bonus one can specify fontsize as an int, but I do not think that warrants a release note. Although it is mentioned in the documentation.
Release Notes
NO ENTRY