#17170 closed defect (fixed)
Sagenb graphics displayhook
Reported by:  vbraun  Owned by:  

Priority:  blocker  Milestone:  sage6.4 
Component:  graphics  Keywords:  
Cc:  Merged in:  
Authors:  Volker Braun  Reviewers:  KarlDieter Crisman, Punarbasu Purkayastha 
Report Upstream:  N/A  Work issues:  
Branch:  76b191a (Commits, GitHub, GitLab)  Commit:  
Dependencies:  Stopgaps: 
Description
The displayhook does not do the right thing in the sagenb notebook.
Change History (20)
comment:1 Changed 7 years ago by
 Priority changed from major to blocker
comment:2 Changed 7 years ago by
 Branch set to u/vbraun/sagenb_graphics_displayhook
comment:3 Changed 7 years ago by
 Commit set to 76b191a978a580a2d78e2b4c1fa55854e7261319
 Status changed from new to needs_review
New commits:
76b191a  Fix SageNB graphics

comment:4 Changed 7 years ago by
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 7 years ago by
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 followup: ↓ 7 Changed 7 years ago by
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(1x,(x,0,1))
, evaluate, and then go back to that same cell and change it to something else likeplot(1x^2,(x,0,1))
, the graphic changes. But:  Try doing a Jsmol, and then try changing *that* notebook cell and reevaluating. 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 sage0size500155928955.jmol.zip sage0size500.jmol $ ls .sage/sage_notebook.sagenb/home/admin/264/cells/82 sage0size500938371752.jmol.zip sage0size500.jmol $ ls .sage/sage_notebook.sagenb/home/admin/264/cells/82 sage0size500522166844.jmol.zip sage0size500.jmol $ ls .sage/sage_notebook.sagenb/home/admin/264/cells/82 sage0size500996347389.jmol.zip sage0size500.jmol
but I suspect that something is not being overwritten somewhere.
comment:7 in reply to: ↑ 6 Changed 7 years ago by
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 followup: ↓ 9 Changed 7 years ago by
comment:9 in reply to: ↑ 8 Changed 7 years ago by
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 7 years ago by
 Reviewers set to KarlDieter 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.
comment:11 Changed 7 years ago by
 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 7 years ago by
Umm... but the big unbreakable rule? I could apply that argument to LOTS of files that need to be rewritten!
comment:13 followup: ↓ 14 Changed 7 years ago by
See #16996
comment:14 in reply to: ↑ 13 Changed 7 years ago by
See #16996
Do you mean this comment? But note that said ticket hasn't even been opened (see here).
comment:15 Changed 7 years ago by
Feel free to open a ticket...
comment:16 followup: ↓ 17 Changed 7 years ago by
 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 7 years ago by
 Reviewers changed from KarlDieter Crisman to KarlDieter 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 7 years ago by
 Branch changed from u/vbraun/sagenb_graphics_displayhook to 76b191a978a580a2d78e2b4c1fa55854e7261319
 Resolution set to fixed
 Status changed from positive_review to closed
comment:19 Changed 7 years ago by
 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 7 years ago by
Probably not needed.
This breaks all sagenb graphics done without
show(...)
, so blocker.