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

Fix get_center method for the jupyter widget and add a get_fov method for both widgets #206

Merged
merged 4 commits into from
May 23, 2019

Conversation

jsub1
Copy link

@jsub1 jsub1 commented May 10, 2019

Part of the work needed to implement issue #200. This uses traitlets to connect the jupyter widget's properties back into the python class.

Copy link
Contributor

@pkgw pkgw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! My only real concern is the periodic timeout needed to update the view info in the Jupyter case. Is there really no way to trigger the update to happen only when the Python code makes a request?

lib/wwt.js Show resolved Hide resolved
pywwt/jupyter.py Outdated Show resolved Hide resolved
@pkgw
Copy link
Contributor

pkgw commented May 10, 2019

Also, the Travis CI failures are due to the "link check" stage of the docs build: see log here. That breakage is unrelated to this PR, but if you can update the relevant URLs while you're at it, that would be good to straighten out.

@pkgw pkgw self-assigned this May 13, 2019
@jsub1
Copy link
Author

jsub1 commented May 13, 2019

I couldn't figure out a way to do it so that the properties are only updated when the python code requests it. Specifically, I can't figure out a way to block the python code until the properties have been update or found to not need updating on the javascript side without just guessing how long that should/could take and sleeping for that much. If we can work that out, we can remove the periodic calls to update_view_data.

@codecov
Copy link

codecov bot commented May 13, 2019

Codecov Report

Merging #206 into master will decrease coverage by 0.35%.
The diff coverage is 28.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #206      +/-   ##
==========================================
- Coverage   58.12%   57.76%   -0.36%     
==========================================
  Files          24       24              
  Lines        1667     1686      +19     
==========================================
+ Hits          969      974       +5     
- Misses        698      712      +14
Impacted Files Coverage Δ
pywwt/jupyter.py 0% <0%> (ø) ⬆️
pywwt/core.py 69.56% <60%> (-0.44%) ⬇️
pywwt/qt.py 74.11% <75%> (+0.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5292e91...a0eec24. Read the comment docs.

@pkgw
Copy link
Contributor

pkgw commented May 14, 2019

OK, mini literature review on the issue of getting data out of the JavaScript. It seems that this is a known issue:

I think that I have a proof-of-concept based on the technique in ipython_blocking that works! Basically, you can block the Python code by manually running the Jupyter event queue and storing events until you see one indicating that your operation has completed. I haven't done a detailed implementation, but the basic approach seems functional.

@astrofrog what do you think? The timeout approach clearly has its weaknesses, but it ought to work pretty well. I certainly am a little wary of manually futzing with Jupyter's event loop but it seems to work ... and I can certainly imagine that other cases might crop up where we'll need the Python code to talk to the JS code, so a more general framework for doing so might be valuable.

@pkgw pkgw force-pushed the get_center-get_fov-work branch from 8a77f4c to a0eec24 Compare May 20, 2019 18:47
@pkgw
Copy link
Contributor

pkgw commented May 20, 2019

I rebased this on wwt/master to hopefully get the CI passing (cf #209).

Copy link
Contributor

@pkgw pkgw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 from me; the issue of the timeout will be punted to #210.

@pkgw pkgw merged commit cee80d8 into WorldWideTelescope:master May 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants