#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, GitHub, GitLab) | Commit: | |
Dependencies: | Stopgaps: |
Description
The displayhook does not do the right thing in the sagenb notebook.
Change History (20)
comment:1 Changed 8 years ago by
Priority: | major → blocker |
---|
comment:2 Changed 8 years ago by
Branch: | → u/vbraun/sagenb_graphics_displayhook |
---|
comment:3 Changed 8 years ago by
Authors: | → Volker Braun |
---|---|
Commit: | → 76b191a978a580a2d78e2b4c1fa55854e7261319 |
Status: | new → needs_review |
New commits:
76b191a | Fix SageNB graphics
|
comment:4 Changed 8 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 8 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 follow-up: 7 Changed 8 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(1-x,(x,0,1))
, evaluate, and then go back to that same cell and change it to something else likeplot(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 Changed 8 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 follow-up: 9 Changed 8 years ago by
comment:9 Changed 8 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 8 years ago by
Reviewers: | → Karl-Dieter Crisman |
---|---|
Status: | needs_review → 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.
Needs work for the doctesting.
comment:11 Changed 8 years ago by
Status: | needs_work → 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 8 years ago by
Umm... but the big unbreakable rule? I could apply that argument to LOTS of files that need to be rewritten!
comment:14 Changed 8 years ago by
See #16996
Do you mean this comment? But note that said ticket hasn't even been opened (see here).
comment:16 follow-up: 17 Changed 8 years ago by
Status: | needs_review → positive_review |
---|
Passes all doctests and plots work again. Why is this not positive review?
comment:17 Changed 8 years ago by
Reviewers: | Karl-Dieter Crisman → 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 8 years ago by
Branch: | u/vbraun/sagenb_graphics_displayhook → 76b191a978a580a2d78e2b4c1fa55854e7261319 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
comment:19 Changed 8 years ago by
Commit: | 76b191a978a580a2d78e2b4c1fa55854e7261319 |
---|
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.
This breaks all sagenb graphics done without
show(...)
, so blocker.