Add sage.misc.latex.pdf to save the image of objects to pdf #38339

kwankyu · 2024-07-08T07:31:47Z

Hence

sage: from sage.misc.latex import pdf
sage: pdf(pi, 'test.pdf')

works. This is a companion to the existing sage.misc.latex.png, which saves png image of objects. Of course, pdf image is scalable and hence more suitable for inclusion into latex documents.

Also we fix it that view() sporadically fails on mac because the temporary file is removed before the viewer opens the file. For example,

sage: view(pi)  # show pi

Thus after this PR, the temporary file is removed when the viewer (Preview app on mac, not the window showing the image) closes. This behavior is consistent with that on linux.

📝 Checklist

The title is concise and informative.
The description explains in detail what this PR is about.
I have linked a relevant issue or discussion.
I have created tests covering the changes.
I have updated the documentation and checked the documentation preview.

⌛ Dependencies

github-actions · 2024-07-08T08:18:14Z

Documentation preview for this PR (built with commit 841298f; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

orlitzky · 2024-08-04T22:45:42Z

sleep(5) will fail randomly, why replace the explicit synchronization?

orlitzky · 2024-08-04T22:48:18Z

The report at https://groups.google.com/g/sage-support/c/YfyaZxc8Z6k has nothing to do with the viewer opening too fast. LaTeX failed to build the PDF in the first place.

kwankyu · 2024-08-05T00:12:42Z

The report at https://groups.google.com/g/sage-support/c/YfyaZxc8Z6k has nothing to do with the viewer opening too fast. LaTeX failed to build the PDF in the first place.

No. The PDF was built. This code

    # Return immediately but only clean up the temporary file after
    # the viewer has closed. This function is synchronous and waits
    # for the process to complete...
    def run_viewer():
        run([viewer, output_file], capture_output=True)
        tmp.cleanup()

does not work as the comment promised. This is what happens: The viewer "opens" (that is, the operating system successfully launched the viewer). Then tmp.cleanup() cleans up the temporary PDF file, before the viewer opens up the PDF file!. The viewer says "no pdf file".

We have to give the viewer enough time to really open the PDF file after it launched. There is no way to detect when the viewer has opened the file. 5 seconds seems enough.

orlitzky · 2024-08-05T00:33:21Z

The error ! ==> Fatal error occurred, no output PDF file produced! comes from LaTeX, not sage. You can run pdflatex -halt-on-error on a file containing junk and that's what it will display.

kwankyu · 2024-08-05T00:45:43Z

I don't know the reporter's system. He says "Latex does work though". So I guess that the message is from the viewer.

kwankyu · 2024-08-05T00:46:59Z

By the way, I also experience this "no pdf file" symptom on my system (mac). Why I prepared this PR :-)

orlitzky · 2024-08-05T00:54:35Z

He tells you his system. Like I said, you can run pdflatex yourself and see exactly where the error comes from. You're trying to fix something that isn't broken.

There may be a problem on mac, but I can't test. You should probably figure out why the current approach doesn't work. Or just add a special case for mac if you don't feel like it. But don't make the situation worse for everyone else because nothing is broken on linux.

kwankyu · 2024-08-05T01:19:56Z

Like I said, you can run pdflatex yourself and see exactly where the error comes from. You're trying to fix something that isn't broken.

I see what you mean. I cannot be sure on the reporter's case.

There may be a problem on mac, but I can't test. You should probably figure out why the current approach doesn't work.

For me, the symptom is sporadic. If the viewer opens up the file quickly enough, no problem. Sometimes not. Maybe dependent on the pdf file size...

Or just add a special case for mac if you don't feel like it.

From the reporter's case, I guessed the symptom can also happen on linux, perhaps depending on the latex system or pdf viewer.

But don't make the situation worse for everyone else because nothing is broken on linux.

OK. No problem. I can wait for input from other people.

dimpase · 2024-08-05T08:21:09Z

how can this happen at all? view is one of many commands using a temporary file, but the typical issue is that these files are left uncleaned

dimpase · 2024-08-05T10:15:09Z

For me, the symptom is sporadic. If the viewer opens up the file quickly enough, no problem. Sometimes not. Maybe dependent on the pdf file size...

Do you by any chance have an antivirus or some other spyware^H^H^^ thing like this installed on your mac? Often that's the case with employer-issued machines...

kwankyu · 2024-08-05T10:39:54Z

No.

I explained how it happens above: #38339 (comment)

There is nothing strange why it happens.

dimpase · 2024-08-05T11:48:50Z

No.

I explained how it happens above: #38339 (comment)

There is nothing strange why it happens.

Do you mean it's a bug in CPython on macOS? Could be, of course, in how threads are handled, but this is not something that should be fixed by adding dodgy extra waiting times, unconditionally on the platform.

Or, perhaps how run_viewer is launched:

    from threading import Thread
    t = Thread(target=run_viewer)
    t.daemon = True
    t.start()

does not work correctly on macOS?

dimpase · 2024-08-05T12:00:05Z

reading https://docs.python.org/3/library/threading.html does not convince me about the validity of the syncronisation via t.daemon used in Sage. It seems to be for something else.

orlitzky · 2024-08-05T12:41:26Z

The daemon flag wasn't for synchronization per se. Without it, sage would refuse to exit while the viewer was still open; sage was waiting for the view/cleanup thread to finish. That page says,

A thread can be flagged as a “daemon thread”. The significance of this flag is that the entire Python program exits when only daemon threads are left. The initial value is inherited from the creating thread. The flag can be set through the daemon property or the daemon constructor argument.

Adding it does allow sage to quit while the viewer is open.

dimpase · 2024-08-05T12:53:26Z

it seems to be the default behaviour of open on macOS is not to wait until the process ends.
However, one can do open -W to let it wait until it completes.

    -W  Causes open to wait until the applications it opens (or that were already open) have exited.  Use with the -n flag to allow open to function as an appropriate app for the $EDITOR
         environment variable.

This way, run() would end only when the viewer completes, and so the files can be cleaned up safely.

orlitzky · 2024-08-05T13:00:13Z

it seems to be the default behaviour of open on macOS is not to wait until the process ends. However, one can do open -W to let it wait until it completes.

This is the way e.g. exo-open works on linux, too. But if adding -W on macos makes the problem go away that's still a great solution.

kwankyu · 2024-08-05T13:58:39Z

Adding -W works. That is, tmp.cleanup() is executed after Preview (the default pdf viewer on macos run by open) closes (this is closing the application, not just closing a window showing the pdf file).

On the other hand, If I end sage session without closing Preview, the thread seems gone and tmp.cleanup() is not executed. I am not sure if the temporary file was cleaned up in this case...

kwankyu · 2024-08-06T06:51:54Z

Now I am using Dima's solution open -W. It is working well on my side.

kwankyu · 2024-08-13T03:29:42Z

Anything else?

The problem of not cleaning up the temporary file if sage session ends while the viewer is open persists, before and after the PR. This PR fixes only the problem on mac that the viewer sporadically crashes because the temporary file is cleaned up too soon.

kwankyu · 2024-08-13T03:30:40Z

and of course, we get a new pdf(obj) command.

dimpase · 2024-08-13T07:41:06Z

Anything else?

The problem of not cleaning up the temporary file if sage session ends while the viewer is open persists, before and after the PR. This PR fixes only the problem on mac that the viewer sporadically crashes because the temporary file is cleaned up too soon.

see #38339 (comment)

kwankyu · 2024-08-13T11:35:33Z

I did. So?

I followed the uncontroversial approach of adding -W to solve the problem on mac. There is no other change regarding the temporary files.

jhpalmieri · 2024-09-30T05:32:23Z

Would it make sense to split the open -W fix into a separate PR?

kwankyu · 2024-09-30T05:37:29Z

Why?

In addition to that, this PR enables

sage: from sage.misc.latex import pdf
sage: pdf(pi, 'test.pdf')

There is no negative review about this.

jhpalmieri · 2024-09-30T17:39:56Z

Why?

In addition to that, this PR enables
sage: from sage.misc.latex import pdf
sage: pdf(pi, 'test.pdf')
There is no negative review about this.

(a) It seems to often be a good idea for a PR to do one thing, not several independent things. Easier for reviewing, easier for merging. (b) In particular, the open -W change fixes a bug and so is somewhat urgent, and it might be faster to get it merged if it's separated from the other part. Or alternatively, it might be faster to get the other contributions merged if the open -W part is the only aspect where there is any disagreement.

kwankyu · 2024-09-30T22:25:57Z

(a) It seems to often be a good idea for a PR to do one thing, not several independent things. Easier for reviewing, easier for merging.

In practice, that argument is reasonable only when the PR is big, and developers often combine small things to the main thing in their PRs.

(b) In particular, the open -W change fixes a bug and so is somewhat urgent, and it might be faster to get it merged if it's separated from the other part.

The "bug" has been in sage for years. I have lived with it. There is nothing urgent for such an old bug. I am not in hurry.

The bug fix is a small thing of this PR. If anyone is in hurry and wants to merge it in quickly, please open a separate PR. It is just one-liner. I am not willing to do it at this moment.

Or alternatively, it might be faster to get the other contributions merged if the open -W part is the only aspect where there is any disagreement.

Though DIma made me confused in #38339 (comment), I think there is no disagreement about that. The fix is not perfect on mac (because of how Preview works), but the situation gets better.

The main thing of this PR is also small. Please review the main thing.

dimpase · 2024-09-30T23:16:01Z

in pdf() function, the engine= doc is a bit misleading. It is only formally None, as it in fact gets immediately assigned a value from preferences.
Can you say so in the doc?

kwankyu · 2024-10-01T00:06:05Z

Right. Done.

kwankyu · 2024-10-01T00:20:54Z

.... The fix is not perfect on mac (because of how Preview works), but the situation gets better.

Let me elaborate on this. On mac, this happens when view(image) runs: Preview app opens an window showing the image from a temporary file. The temporary file is supposed to be deleted when the Preview app closes. But a mac user just closes the window containing the image, and the Preview app still lingers. Hence the temporary file won't be deleted until the user closes the Preview app. If the user is like me, she will close sage long before she would close the Preview app (perhaps when she turns the power off :-). I don't know what happens to the temporary file, since sage was closed before the Preview app...

kwankyu · 2024-10-05T13:18:44Z

.... The fix is not perfect on mac (because of how Preview works), but the situation gets better.

... I don't know what happens to the temporary file, since sage was closed before the Preview app...

I experimented more. The temporary file is removed when sage is closed, even if the Preview app is still running. Perfect!

dimpase · 2024-10-05T13:55:49Z

I'd happy to give it a positive review.

kwankyu · 2024-10-05T22:30:46Z

Thanks!

…s to pdf       Hence ```sage sage: from sage.misc.latex import pdf sage: pdf(pi, 'test.pdf') ``` works. This is a companion to the existing `sage.misc.latex.png`, which saves png image of objects. Of course, pdf image is scalable and hence more suitable for inclusion into latex documents. Also we fix it that `view()` sporadically fails on mac because the temporary file is removed before the viewer opens the file. For example, ```sage sage: view(pi) # show pi ``` Thus after this PR, the temporary file is removed when the viewer (`Preview` app on mac, not the window showing the image) closes. This behavior is consistent with that on linux. ### 📝 Checklist  - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies    URL: sagemath#38339 Reported by: Kwankyu Lee Reviewer(s): Dima Pasechnik

Fixes sagemath#38745 ### 📝 Checklist  - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies    sagemath#38339 URL: sagemath#38764 Reported by: Kwankyu Lee Reviewer(s): Travis Scrimshaw

kwankyu changed the title ~~Add pdf()~~ Add sage.misc.latex.pdf to save sage objects to pdf file Jul 8, 2024

kwankyu changed the title ~~Add sage.misc.latex.pdf to save sage objects to pdf file~~ Add sage.misc.latex.pdf to save the pdf image of objects Jul 8, 2024

kwankyu changed the title ~~Add sage.misc.latex.pdf to save the pdf image of objects~~ Add sage.misc.latex.pdf to save the image of objects to pdf Jul 8, 2024

kwankyu marked this pull request as ready for review July 8, 2024 12:54

github-actions bot added s: needs review v: small labels Jul 8, 2024

mkoeppe requested a review from seblabbe July 10, 2024 01:20

mkoeppe added the c: graphics label Jul 10, 2024

kwankyu force-pushed the p/add-pdf branch from 5a94a77 to 6ee280b Compare August 4, 2024 10:43

mkoeppe requested a review from orlitzky August 4, 2024 20:17

kwankyu added s: needs info and removed s: needs review labels Aug 5, 2024

github-actions bot added s: needs review and removed s: needs info labels Aug 6, 2024

anneschilling mentioned this pull request Sep 30, 2024

Problem with view on MacOS #38745

Open

2 tasks

Add pdf()

841298f

kwankyu force-pushed the p/add-pdf branch from 086c37e to 841298f Compare October 4, 2024 09:25

kwankyu mentioned this pull request Oct 4, 2024

Fix the latex representation of crystals tableaux #38764

Merged

5 tasks

dimpase approved these changes Oct 5, 2024

View reviewed changes

kwankyu added s: positive review and removed s: needs review labels Oct 5, 2024

anneschilling mentioned this pull request Oct 7, 2024

Add method tikz (or latex_standalone) within combinat.crystals module #38759

Open

5 tasks

vbraun merged commit 6b02856 into sagemath:develop Oct 12, 2024
19 of 20 checks passed

github-actions bot removed the s: positive review label Oct 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sage.misc.latex.pdf to save the image of objects to pdf #38339

Add sage.misc.latex.pdf to save the image of objects to pdf #38339

kwankyu commented Jul 8, 2024 •

edited

Loading

github-actions bot commented Jul 8, 2024 •

edited

Loading

orlitzky commented Aug 4, 2024

orlitzky commented Aug 4, 2024

kwankyu commented Aug 5, 2024 •

edited

Loading

orlitzky commented Aug 5, 2024

kwankyu commented Aug 5, 2024

kwankyu commented Aug 5, 2024 •

edited

Loading

orlitzky commented Aug 5, 2024

kwankyu commented Aug 5, 2024

dimpase commented Aug 5, 2024

dimpase commented Aug 5, 2024

kwankyu commented Aug 5, 2024

dimpase commented Aug 5, 2024

dimpase commented Aug 5, 2024

orlitzky commented Aug 5, 2024

dimpase commented Aug 5, 2024

orlitzky commented Aug 5, 2024

kwankyu commented Aug 5, 2024

kwankyu commented Aug 6, 2024

kwankyu commented Aug 13, 2024

kwankyu commented Aug 13, 2024

dimpase commented Aug 13, 2024

kwankyu commented Aug 13, 2024

jhpalmieri commented Sep 30, 2024

kwankyu commented Sep 30, 2024

jhpalmieri commented Sep 30, 2024

kwankyu commented Sep 30, 2024

dimpase commented Sep 30, 2024

kwankyu commented Oct 1, 2024

kwankyu commented Oct 1, 2024

kwankyu commented Oct 5, 2024

dimpase commented Oct 5, 2024

kwankyu commented Oct 5, 2024

Add sage.misc.latex.pdf to save the image of objects to pdf #38339

Add sage.misc.latex.pdf to save the image of objects to pdf #38339

Conversation

kwankyu commented Jul 8, 2024 • edited Loading

📝 Checklist

⌛ Dependencies

github-actions bot commented Jul 8, 2024 • edited Loading

orlitzky commented Aug 4, 2024

orlitzky commented Aug 4, 2024

kwankyu commented Aug 5, 2024 • edited Loading

orlitzky commented Aug 5, 2024

kwankyu commented Aug 5, 2024

kwankyu commented Aug 5, 2024 • edited Loading

orlitzky commented Aug 5, 2024

kwankyu commented Aug 5, 2024

dimpase commented Aug 5, 2024

dimpase commented Aug 5, 2024

kwankyu commented Aug 5, 2024

dimpase commented Aug 5, 2024

dimpase commented Aug 5, 2024

orlitzky commented Aug 5, 2024

dimpase commented Aug 5, 2024

orlitzky commented Aug 5, 2024

kwankyu commented Aug 5, 2024

kwankyu commented Aug 6, 2024

kwankyu commented Aug 13, 2024

kwankyu commented Aug 13, 2024

dimpase commented Aug 13, 2024

kwankyu commented Aug 13, 2024

jhpalmieri commented Sep 30, 2024

kwankyu commented Sep 30, 2024

jhpalmieri commented Sep 30, 2024

kwankyu commented Sep 30, 2024

dimpase commented Sep 30, 2024

kwankyu commented Oct 1, 2024

kwankyu commented Oct 1, 2024

kwankyu commented Oct 5, 2024

dimpase commented Oct 5, 2024

kwankyu commented Oct 5, 2024

kwankyu commented Jul 8, 2024 •

edited

Loading

github-actions bot commented Jul 8, 2024 •

edited

Loading

kwankyu commented Aug 5, 2024 •

edited

Loading

kwankyu commented Aug 5, 2024 •

edited

Loading