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

Make Imviz panning more seamless #864

Merged
merged 2 commits into from
Sep 15, 2021

Conversation

astrofrog
Copy link
Collaborator

@astrofrog astrofrog commented Sep 14, 2021

Description

This pull request uses the option added in glue-viz/glue-jupyter#246 to expand the image fixed resolution buffer beyond the edge of the axes, making panning more seamless:

Peek 2021-09-14 16-00

This is IMHO an improvement over the default behavior right now but it is not perfect - if one pans too fast one might still reach the 'edge' of the buffer and have to wait for it to refresh. We can make the padding larger but then it will take longer to compute, so we need to find the right balance.

There is another - separate - improvement I would like to make which is to only compute the buffer when the mouse button is released during panning, instead of every 0.5 seconds during panning, but this will be done in a separate PR and may not happen before the MVP deadline, so I wanted to at least get this quick fix in.

The glue-jupyter v0.9 release is in progress so this currently requires glue-jupyter dev, but in <1h it should work fine (and the CI can be restarted once glue-jupyter v0.9 is visible on PyPI)

Fixes #564

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

  • Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
  • Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
  • Do the proposed changes follow the STScI Style Guides?
  • Are tests added/updated as required? If so, do they follow the STScI Style Guides?
  • Are docs added/updated as required? If so, do they follow the STScI Style Guides?
  • Did the CI pass? If not, are the failures related?
  • Is a change log needed? If yes, is it added to CHANGES.rst?
  • Is a milestone set? Milestone is only currently required for PRs related to Imviz MVP.
  • After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

@pllim
Copy link
Contributor

pllim commented Sep 14, 2021

So, is this still going to happen?

#564 (comment)

@pllim
Copy link
Contributor

pllim commented Sep 14, 2021

Going to restart the jobs so they pick up new glue-jupyter release.

@pllim pllim closed this Sep 14, 2021
@pllim pllim reopened this Sep 14, 2021
@astrofrog
Copy link
Collaborator Author

So, is this still going to happen?

As noted in the description above - not in this PR. I don't think this is specifically MVP critical if what is in this PR is good enough.

@astrofrog
Copy link
Collaborator Author

glue-jupyter v0.9 is out for the record, so this should work properly now.

@pllim

This comment has been minimized.

@pllim pllim closed this Sep 14, 2021
@pllim pllim reopened this Sep 14, 2021
@pllim
Copy link
Contributor

pllim commented Sep 14, 2021

As noted in the description above - not in this PR. I don't think this is specifically MVP critical if what is in this PR is good enough.

I am just not sure if I should open a follow-up issue for it for just forget about it.

@codecov
Copy link

codecov bot commented Sep 14, 2021

Codecov Report

Merging #864 (6349029) into main (36605e8) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #864   +/-   ##
=======================================
  Coverage   68.29%   68.30%           
=======================================
  Files          65       65           
  Lines        4741     4742    +1     
=======================================
+ Hits         3238     3239    +1     
  Misses       1503     1503           
Impacted Files Coverage Δ
jdaviz/configs/imviz/plugins/viewers.py 41.46% <100.00%> (+0.72%) ⬆️

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 36605e8...6349029. Read the comment docs.

@astrofrog
Copy link
Collaborator Author

I won't forget about it but feel free to open an issue at glue-jupyter 😀

@pllim
Copy link
Contributor

pllim commented Sep 14, 2021

Care to rebase to resolve conflict? 😅

@pllim
Copy link
Contributor

pllim commented Sep 14, 2021

I rebased locally and am using glue-jupyter 0.9, but I cannot say I can see much difference compared to what is already in main. I am interested to see if @rosteen , @javerbukh , or @ojustino feel otherwise.

@pllim
Copy link
Contributor

pllim commented Sep 14, 2021

I guess I like to pan too much around 47-Tuc. Maybe someone with a more realistic science use case will be happy enough with this... 🤷

@astrofrog
Copy link
Collaborator Author

Just for ease of comparison:

Before

Peek 2021-09-14 22-32

After

Peek 2021-09-14 16-00

Copy link
Contributor

@pllim pllim left a comment

Choose a reason for hiding this comment

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

I guess it doesn't hurt... 😸

Copy link
Contributor

@ojustino ojustino left a comment

Choose a reason for hiding this comment

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

Side by side, I do notice that there's more area over which you can pan before the lag appears. People who pan aggressively won't notice much, but I think this is a good contribution for those who don't.

@pllim
Copy link
Contributor

pllim commented Sep 15, 2021

Thanks, all!

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

Successfully merging this pull request may close these issues.

Imviz pan/zoom needs improvements (lag in pan)
3 participants