robert-dodier / maxima-jupyter

A Maxima kernel for Jupyter, based on CL-Jupyter (Common Lisp kernel)
Other
185 stars 31 forks source link

Race condition with gnuplot_pipes #122

Closed mrohne closed 11 months ago

mrohne commented 11 months ago

Examples using plot2d results in examples/Plots.ipynb not working.

Observations: The (default) gnuplot_pipes driver has no provision for maxima to wait for the plotting to finish. At the time of MAXIMA-JUPYTER::PLOT-P the plot file is not even there (no plot shown), or the plot file is incomplete during MAXIMA::JUPYTER-FILE (broken plot icon shown).

Platforms:

Workaround: Use set_plot_option([plot_format,gnuplot])$

robert-dodier commented 11 months ago

Thanks for your comment. From what you were saying, it makes sense to set the default plot_format to gnuplot instead of gnuplot_pipes. Does that sound right to you? If so, I will commit that change. @yitzchak Do you foresee any complications if plot_format is changed?

mrohne commented 11 months ago

Thank you for looking into this! I agree, setting plot_options to gnuplot seems to work on the systems I have access to.

Ole

On Mon, Sep 18, 2023, 20:53 Robert Dodier @.***> wrote:

Thanks for your comment. From what you were saying, it makes sense to set the default plot_format to gnuplot instead of gnuplot_pipes. Does that sound right to you? If so, I will commit that change. @yitzchak https://github.com/yitzchak Do you foresee any complications if plot_format is changed?

— Reply to this email directly, view it on GitHub https://github.com/robert-dodier/maxima-jupyter/issues/122#issuecomment-1724194218, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABF7J7HTIAJCTGN3ZCHMSLX3CKDBANCNFSM6AAAAAA44EMSM4 . You are receiving this because you authored the thread.Message ID: @.***>

yitzchak commented 11 months ago

Sounds good to me.

robert-dodier commented 11 months ago

Hmm, I am having trouble creating an embedded plot. I am working on Ubuntu 16.04 + SBCL 2.3.7 + Maxima post-5.47, with "loaded by Quicklisp" installation method. I launched the notebook. When I tried a plot2d example, I got a separate Gnuplot window, but I expected the image to be embedded in the notebook. I also tried set_plot_option ([svg_file, "tmpfooplot.svg"]); and then plot2d again, but it just returns the name of the SVG file. I tried set_plot_option ([plot_format, gnuplot]);, and then plot2d again, now it returns the name of the Gnuplot output file and the name of the SVG file. Not sure what to try at this point.

It is a bit embarassing, but I don't remember how to enable embedded plot images; can someone @mrohne @yitzchak remind me? Thanks for your help.

mrohne commented 11 months ago

Sorry I forgot about this: In stock Maxima, gnuplot and gnuplot_pipes disagree about what return value to pass from plot2d. I had patched the gnuplot-driver to match gnuplot_pipes and only return the plot file, which is what maxima-jupyter expects. In the end I'm afraid there is no user-level workaround here. Ole

On Wed, Sep 20, 2023, 22:03 Robert Dodier @.***> wrote:

Hmm, I am having trouble creating an embedded plot. I am working on Ubuntu 16.04 + SBCL 2.3.7 + Maxima post-5.47, with "loaded by Quicklisp" installation method. I launched the notebook. When I tried a plot2d example, I got a separate Gnuplot window, but I expected the image to be embedded in the notebook. I also tried set_plot_option ([svg_file, "tmpfooplot.svg"]); and then plot2d again, but it just returns the name of the SVG file. I tried set_plot_option ([plot_format, gnuplot]);, and then plot2d again, now it returns the name of the Gnuplot output file and the name of the SVG file. Not sure what to try at this point.

It is a bit embarassing, but I don't remember how to enable embedded plot images; can someone @mrohne https://github.com/mrohne @yitzchak https://github.com/yitzchak remind me? Thanks for your help.

— Reply to this email directly, view it on GitHub https://github.com/robert-dodier/maxima-jupyter/issues/122#issuecomment-1728351346, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABF7J3SYNGBKBTL2MMTVDLX3NDYBANCNFSM6AAAAAA44EMSM4 . You are receiving this because you were mentioned.Message ID: @.***>

robert-dodier commented 11 months ago

What is your patch for gnuplot-driver? I have been trying some different modifications and I can't seem to find something that works. I'll be interested to see what you did.

yitzchak commented 11 months ago

I am thinking we just need to look at the plot options in the plot2d override in order to decide what to do. We do something like that in implicit_plot already.

https://github.com/robert-dodier/maxima-jupyter/blob/f12f6ef977778db717844c7332ba374d555758e9/src/overrides.lisp#L116-L120

yitzchak commented 11 months ago

Also, in the gnuplot_pipes situation we could spawn a thread to wait on the result and then just use Jupyter display messages versus relying on the return value.

robert-dodier commented 11 months ago

@mrohne I have merged a PR from Tarn Burton (commit e20fdd7a) which is intended to fix this issue. Ole, can I ask you to try the new version and verify whether it works for you?

mrohne commented 11 months ago

Sorry I missed the question above about the patch to gnuplot. I guess this is moot now. For the record, I copied the return logic from gnuplot_pipes (below).

diff --git a/src/gnuplot_def.lisp b/src/gnuplot_def.lisp index 80c174bd5..4e08d7ce2 100644 --- a/src/gnuplot_def.lisp +++ b/src/gnuplot_def.lisp @@ -725,7 +725,8 @@ :direction :output :if-exists :supersede) (format fl "~a" (slot-value plot 'data))) (gnuplot-process options file output-file)

On 21 Sep 2023, at 07:09, Robert Dodier @.***> wrote:

What is your patch for gnuplot-driver? I have been trying some different modifications and I can't seem to find something that works. I'll be interested to see what you did.

— Reply to this email directly, view it on GitHub https://github.com/robert-dodier/maxima-jupyter/issues/122#issuecomment-1728840910, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABF7J6Z5DLETGITNKN26FDX3PDXVANCNFSM6AAAAAA44EMSM4. You are receiving this because you were mentioned.

mrohne commented 11 months ago

@mrohne I have merged a PR from Tarn Burton (commit e20fdd7) which is intended to fix this issue. Ole, can I ask you to try the new version and verify whether it works for you?

I confirm this patch works, assuming set_plot_option([plot_format,gnuplot])$

robert-dodier commented 11 months ago

Ole, thanks for the update, sounds good.

@yitzchak What do you think about making gnuplot the default plot_format? I guess I am thinking that could be helpful, since otherwise people might run into the same race condition, which has a non-obvious workaround.

yitzchak commented 11 months ago

I think that is a good idea. I don't think the gnuplot_pipes method was really designed to output files synchronously.