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

WIP: Split plotting backends, default to Plotly #481

Closed
wants to merge 14 commits into from

Conversation

Projects
None yet
2 participants
@Juanlu001
Copy link
Member

Juanlu001 commented Oct 28, 2018

Addresses #338.

Now trying to import OrbitPlotter directly from poliastro.plotting
fails, because it's the "old" backend and users are encouraged to
use OrbitPlotter2D and OrbitPlotter3D instead, which use plotly
and not matplotlib behind the scenes.

This does not address:

  • Updates in documentation
  • Failing test because OrbitPlotter2D has no set_frame method (#480)
  • Test simplification and refactoring (#448 and #447)

Also, notice that #281 should be fixed first.

@Juanlu001
Copy link
Member Author

Juanlu001 left a comment

I reject my own PR, see comment.

Show resolved Hide resolved src/poliastro/plotting/common.py Outdated

@Juanlu001 Juanlu001 force-pushed the Juanlu001:split-plotting branch from b98550d to 886b892 Nov 3, 2018

@Juanlu001

This comment has been minimized.

Copy link
Member Author

Juanlu001 commented Nov 3, 2018

The OrbitPlotter* code is so ugly (entirely my fault) that I can't wait to refactor it all :) However, for that I'd like to wait for better reference frame support: #434, which should arrive for the next release.

OrbitPlotter2D has incomplete redraw support (a set_frame call will fail with a NotImplementedError) but at least it now plots things properly, which it wasn't doing before (fix #483).

This PR still needs to update the documentation to be mergeable.

@Juanlu001

This comment has been minimized.

Copy link
Member Author

Juanlu001 commented Nov 3, 2018

Also, I might want to use this opportunity to just use FigureWidgets everywhere, forget about init_notebook_mode, fix #281 by accident, and start a new family of problems because of widgets handling in Jupyter notebook and lab...

@Juanlu001

This comment has been minimized.

Copy link
Member Author

Juanlu001 commented Nov 3, 2018

Rebase after #487 is merged.

@shreyasbapat

This comment has been minimized.

Copy link
Member

shreyasbapat commented Nov 3, 2018

There has to be lot of updates to the documentation and other places. Will you be doing it? or Can I help? 😄

Juanlu001 added some commits Oct 28, 2018

WIP: Split plotting backends, default to Plotly
Addresses #338.

Now trying to import OrbitPlotter directly from poliastro.plotting
fails, because it's the "old" backend and users are encouraged to
use OrbitPlotter2D and OrbitPlotter3D instead, which use plotly
and not matplotlib behind the scenes.

This does not address:

* Updates in documentation
* Failing test because OrbitPlotter2D has no set_frame method (#480)
* Test simplification and refactoring (#448 and #447)

Also, notice that #281 should be fixed first.
@Juanlu001

This comment has been minimized.

Copy link
Member Author

Juanlu001 commented Nov 9, 2018

I did a test with plotly widgets with this PR and it's not ready yet: there are lots of workflows that fail, and updating the widget just does not happen. This requires some thought.

@Juanlu001 Juanlu001 force-pushed the Juanlu001:split-plotting branch from f39838e to c67cd4a Nov 9, 2018

@Juanlu001

This comment has been minimized.

Copy link
Member Author

Juanlu001 commented Nov 9, 2018

On the other hand, I tried embedding a widget in the docs inspired by ipyleaflet and it worked beautifully, at least in my local computer. However, there's a strange effect on hover:

hover-strange

@Juanlu001

This comment has been minimized.

Copy link
Member Author

Juanlu001 commented Nov 10, 2018

I fear I will have a hard time trying to concentrate and finish this PR soon: partial reprojection is not enough (some tests are failing because of that) and probably a change in architecture will be needed for this to be successful. I promise this will be done before Christmas, but can't do any better.

@shreyasbapat shreyasbapat referenced this pull request Nov 11, 2018

Closed

Unify plotting tests #448

@Juanlu001

This comment has been minimized.

Copy link
Member Author

Juanlu001 commented Dec 26, 2018

I promise this will be done before Christmas, but can't do any better.

Well... 😅

I added some notes about my current thinking at #338 (comment). I am closing this until I have a better idea of what to do.

@Juanlu001 Juanlu001 closed this Dec 26, 2018

@wafflebot wafflebot bot removed the 2 - In Progress label Dec 26, 2018

@Juanlu001 Juanlu001 referenced this pull request Jan 4, 2019

Merged

Refactor plotting #528

6 of 8 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.