Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't use installed_packages() for threejs URL #25665

Closed
egourgoulhon opened this issue Jun 26, 2018 · 13 comments
Closed

Don't use installed_packages() for threejs URL #25665

egourgoulhon opened this issue Jun 26, 2018 · 13 comments

Comments

@egourgoulhon
Copy link
Member

Since Sage 8.3.beta3, running the command

show(sphere(), viewer='threejs', online=True)

in a Jupyter notebook displays nothing. It was fine in Sage <= 8.3.beta2.
Note that the option online=True is required by tools like http://nbviewer.jupyter.org/ and CoCalc.

It seems that the issue is due to the following change introduced in #25040 (which has been merged in Sage 8.3.beta3):

sage: installed_packages()['threejs']
'r80.p0'

while in Sage <= 8.3.beta2, one has

sage: installed_packages()['threejs']
'r80'

Indeed the value of installed_packages()['threejs'] is used to form the url in lines 749-750 of src/sage/repl/rich_output/display_manager.py

CC: @embray @jdemeyer @antonio-rojas @kiwifb

Component: graphics

Keywords: threejs

Author: Frédéric Chapoton

Branch/Commit: 867e1ff

Reviewer: Eric Gourgoulhon

Issue created by migration from https://trac.sagemath.org/ticket/25665

@egourgoulhon egourgoulhon added this to the sage-8.3 milestone Jun 26, 2018
@jdemeyer
Copy link

Replying to @egourgoulhon:

Indeed the value of installed_packages()['threejs'] is used to form the url in lines 749-750 of src/sage/repl/rich_output/display_manager.py

That's a really bad idea for multiple reasons.

@jdemeyer

This comment has been minimized.

@jdemeyer jdemeyer changed the title threejs viewer broken after Sage 8.3.beta3 Don't use installed_packages() for threejs URL Jun 26, 2018
@egourgoulhon

This comment has been minimized.

@egourgoulhon
Copy link
Member Author

comment:5

Replying to @jdemeyer:

Replying to @egourgoulhon:

Indeed the value of installed_packages()['threejs'] is used to form the url in lines 749-750 of src/sage/repl/rich_output/display_manager.py

That's a really bad idea for multiple reasons.

Yes probably. I guess that was to ensure compatibility between the version of threejs actually used by Sage. By the way, in the directory upstream, we have threejs-r80.tar.gz, so where does the .p0 come from?

@novoselt
Copy link
Member

comment:7

That was indeed to ensure that the same version is used. If there is a better way - please implement it! And an obvious way to get rid of patched suffixes for this particular case is to just use the part of the version before the dot.

@fchapoton
Copy link
Contributor

Commit: 867e1ff

@fchapoton
Copy link
Contributor

Author: Frédéric Chapoton

@fchapoton
Copy link
Contributor

New commits:

867e1fftrac 25665 fixing threejs URL

@fchapoton
Copy link
Contributor

Branch: u/chapoton/25665

@kiwifb
Copy link
Member

kiwifb commented Jul 1, 2018

comment:11

The use of is installed_packages at runtime in sage is of course one of my pet hates. I don't have an alternative to offer at the present time. In fact I am just realising how this bit is broken in sage-on-gentoo right now (I don't offer an offline option either). Where could I find a list of valid versions for the online cdn? Is there something like "latest"?

@egourgoulhon
Copy link
Member Author

comment:12

Thanks for the fix!

@egourgoulhon
Copy link
Member Author

Reviewer: Eric Gourgoulhon

@vbraun
Copy link
Member

vbraun commented Jul 7, 2018

Changed branch from u/chapoton/25665 to 867e1ff

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants