Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#17170 closed defect (fixed)

Sagenb graphics displayhook

Reported by: vbraun Owned by:
Priority: blocker Milestone: sage-6.4
Component: graphics Keywords:
Cc: Merged in:
Authors: Volker Braun Reviewers: Karl-Dieter Crisman, Punarbasu Purkayastha
Report Upstream: N/A Work issues:
Branch: 76b191a (Commits) Commit:
Dependencies: Stopgaps:

Description

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

Change History (20)

comment:1 Changed 5 years ago by kcrisman

  • Priority changed from major to blocker

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

comment:2 Changed 5 years ago by vbraun

  • Branch set to u/vbraun/sagenb_graphics_displayhook

comment:3 Changed 5 years ago by vbraun

  • Authors set to Volker Braun
  • Commit set to 76b191a978a580a2d78e2b4c1fa55854e7261319
  • Status changed from new to needs_review

New commits:

76b191aFix SageNB graphics

comment:4 Changed 5 years ago by 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.

comment:5 Changed 5 years ago by 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 :)

comment:6 follow-up: Changed 5 years ago by 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.

comment:7 in reply to: ↑ 6 Changed 5 years ago by 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.

comment:8 follow-up: Changed 5 years ago by 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.

comment:9 in reply to: ↑ 8 Changed 5 years ago by 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.

comment:10 Changed 5 years ago by kcrisman

  • Reviewers set to Karl-Dieter Crisman
  • Status changed from needs_review to needs_work

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 - especially it would be helpful for people who have NOT been involved with the jsmol transition to just confirm that this doesn't cause caching problems for 3d plots in the current Sage.

Needs work for the doctesting.

Last edited 5 years ago by kcrisman (previous) (diff)

comment:11 Changed 5 years ago by vbraun

  • Status changed from needs_work to needs_review

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

comment:12 Changed 5 years ago by kcrisman

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

comment:13 follow-up: Changed 5 years ago by vbraun

See #16996

comment:14 in reply to: ↑ 13 Changed 5 years ago by kcrisman

See #16996

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

comment:15 Changed 5 years ago by vbraun

Feel free to open a ticket...

comment:16 follow-up: Changed 5 years ago by ppurka

  • Status changed from needs_review to positive_review

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

comment:17 in reply to: ↑ 16 Changed 5 years ago by kcrisman

  • Reviewers changed from Karl-Dieter Crisman to Karl-Dieter Crisman, Punarbasu Purkayastha

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.

comment:18 Changed 5 years ago by vbraun

  • Branch changed from u/vbraun/sagenb_graphics_displayhook to 76b191a978a580a2d78e2b4c1fa55854e7261319
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:19 Changed 5 years ago by jdemeyer

  • Commit 76b191a978a580a2d78e2b4c1fa55854e7261319 deleted

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.

comment:20 Changed 5 years ago by vbraun

Probably not needed.

Note: See TracTickets for help on using tickets.