Sagenb graphics displayhook

[vbraun]

The displayhook does not do the right thing in the sagenb notebook.

[kcrisman]

This breaks all sagenb graphics done without show(...), so blocker.

[vbraun]

New commits:

​76b191a

Fix SageNB graphics

[kcrisman]

Seems to work properly in command line and for 2d in the notebook. It'll be a moment before I can try with 3d (at least jmol, didn't think about tachyon) with the union of this and the new jmol, though the "static" image turned up okay, so I guess this works that way too.

Doesn't

if isinstance(obj, SageObject) and hasattr(obj, '_graphics_'):

cover both 2d and 3d graphics?

Also, you probably should do at least a minimal doctest for sagenb_embedding and save_as.

Finally, what with all the changes to display stuff I think that it would be well worth asking for a little extra testing of any rc for this series visually.

[kcrisman]

Also, you should probably have someone test on Linux and definitely have someone test who hasn't been messing around with different sagenb branches and possibly got something mixed up because of that :)

[kcrisman]

Hmm, I have a question here. Probably due to the change in the way 3d is handled?

• If you make a standard 2d plot like plot(1-x,(x,0,1)), evaluate, and then go back to that same cell and change it to something else like plot(1-x^2,(x,0,1)), the graphic changes. But:
• Try doing a Jsmol, and then try changing *that* notebook cell and re-evaluating. If you don't have the "Load 3D Live" checkbox clicked, you'll think you changed it - but you didn't! As soon as you make it live, it's the exact same one as before. If you have the live checkbox clicked (this refers to #16004, for those who come new to this) then this is quite obvious.

If it helps, the filenames are definitely changing:

$ ls .sage/sage_notebook.sagenb/home/admin/264/cells/82
sage0-size500-155928955.jmol.zip	sage0-size500.jmol
$ ls .sage/sage_notebook.sagenb/home/admin/264/cells/82
sage0-size500-938371752.jmol.zip	sage0-size500.jmol
$ ls .sage/sage_notebook.sagenb/home/admin/264/cells/82
sage0-size500-522166844.jmol.zip	sage0-size500.jmol
$ ls .sage/sage_notebook.sagenb/home/admin/264/cells/82
sage0-size500-996347389.jmol.zip	sage0-size500.jmol

but I suspect that something is not being overwritten somewhere.

[kcrisman]

Hmm, I have a question here. Probably due to the change in the way 3d is handled?

Okay, I can confirm this problem is unrelated to this change.

[gutow]

This is very odd. My test of Jmol/JSmol does not reproduce the problem you report. I am using the latest from #17020 and #16004. Something must be out of wack.

[kcrisman]

Hmm, I have a question here. Probably due to the change in the way 3d is handled?

Okay, I can confirm this problem is unrelated to this change.

This is very odd. My test of Jmol/JSmol does not reproduce the problem you report. I am using the latest from #17020 and #16004. Something must be out of wack.

Is it using this ticket, I assume? If you're not using the latest development Sage + this ticket, and you are NOT experiencing this problem, then maybe this ticket is the problem after all, though I thought I had ruled it out.

[kcrisman]

I have a feeling that this patch has exposed a latent caching issue (see this comment) in the way the notebook handles the script that actually launches the jmol. I believe I have two different fixes at that ticket (here and here), so I think we can go ahead and have people review this.

Needs work for the doctesting.

[vbraun]

I'm not going to add doctests to graphics_file.py, that needs to be rewritten in a future ticket anyways.

[kcrisman]

Umm... but the big unbreakable rule? I could apply that argument to LOTS of files that need to be rewritten!

[vbraun]

See #16996

[kcrisman]

See #16996

Do you mean this comment? But note that said ticket hasn't even been opened (see here).

[vbraun]

Feel free to open a ticket...

[ppurka]

Passes all doctests and plots work again. Why is this not positive review?

[kcrisman]

Passes all doctests and plots work again. Why is this not positive review?

Because we needed a reviewer, and I didn't have time to actually read the code in detail in this area I am less familiar with. But several others have tested it as well.

[jdemeyer]

Do we need the

if sage.doctest.DOCTEST_MODE:
    return

lines in sagenb_embedding? I'm removing them in #16640 and it doesn't seem to break anything.

[vbraun]

Probably not needed.