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

Added dragging for dynamic spectrum updating. #707

Merged
merged 2 commits into from
Sep 9, 2015
Merged

Added dragging for dynamic spectrum updating. #707

merged 2 commits into from
Sep 9, 2015

Conversation

stscieisenhamer
Copy link
Contributor

Just because it was dirt simple, added dynamic spectrum extraction while dragging ROI in the spectrum tool.

@ChrisBeaumont
Copy link
Member

Build failure seems unrelated.

Will this hurt usability for larger datasets with a noticeable lag when updating the spectrum?

@stscieisenhamer
Copy link
Contributor Author

Good point about large datasets. Maybe makes sense that it follows the same modifier key as the selection dragging?

And no clue as to the build issue. But can investigate if no one else has a clue either.

@astrofrog
Copy link
Member

@stscieisenhamer - yes, I think for consistency we should use the same modifier key here.

@stscieisenhamer
Copy link
Contributor Author

(not) Thinking out loud and attempting to generalize this a bit: If Control-drag == scrub, then maybe a double modifier, such as Shift-Control-Drag == scrub-with-update.

@ChrisBeaumont
Copy link
Member

Just throwing ideas out there: I seem to recall gaia (the spectral datacube viewer, not the telescope) had a checkbox that toggled whether dragging an ROI triggered continuous auto-updating or not. Ginga uses a lot of keyboard modifiers, but they tend to display status messages along the lines of "hold down control to auto-update".

@ChrisBeaumont
Copy link
Member

The build failures seem unrelated, so I wouldn't worry about them

@astrofrog
Copy link
Member

@stscieisenhamer - can you rebase? This should fix the failures. The rebase should be easy since there are no conflicts.

@stscieisenhamer
Copy link
Contributor Author

@astrofrog So, to completely show how I mess this up: As per standard procedure, I do the fetch and the rebase:

$ git checkout extractor_drag
$ git status <<< even with origin/extractor_drag
$ fetch upstream
$ rebase -i upstream/master
<<<<Only one commit, finish rebase
$ git status
Your branch and 'origin/extractor_drag' have diverged,
and have 109 and 1 different commit each, respectively.

What I had been doing was a git pull at this point. However, methinks there are other options, which I find two: either do a forced push or a git pull --rebase

Thoughts?

@astrofrog
Copy link
Member

@stscieisenhamer - what is the output of git log? (the ~20 first lines)

@stscieisenhamer
Copy link
Contributor Author

commit 28c08469df583a63c2932f8455bf2bb97abf9f48
Author: J.D.Eisenhamer <eisenhamer@stsci.edu>
Date:   Fri Jul 17 07:17:14 2015 -0400

    Added dragging for dynamic spectrum updating.

commit 62814f053ed6e44d7acc7e26441edeac12300b77
Merge: 69f6b16 2f5eca6
Author: Thomas Robitaille <thomas.robitaille@gmail.com>
Date:   Thu Sep 3 12:26:03 2015 +0200

    Merge pull request #732 from astrofrog/fits-containers

    FITS containers [rebased]

commit 2f5eca66f4cbe18ba114668819ec2c6317f7b7c8
Author: Thomas Robitaille <thomas.robitaille@gmail.com>
Date:   Thu Sep 3 10:06:15 2015 +0200

    Use ordered dictionaries to make sure order is deterministic (important when loading in old session files)

commit 00287b8b274e5ba156053fed7e5b38e36916a96c
Author: Thomas Robitaille <thomas.robitaille@gmail.com>
Date:   Wed Sep 2 23:45:15 2015 +0200

    Added changelog entry

commit 6b629a4fa0cfec29212ef73562e70f5138ac872b
Author: Thomas Robitaille <thomas.robitaille@gmail.com>
Date:   Wed Aug 26 09:56:59 2015 +0100

    Fix older FITS tests

commit 9634660b3c52bff45e0c7ae1ef94b633b35c776a
Author: Thomas Robitaille <thomas.robitaille@gmail.com>
Date:   Wed Aug 26 09:46:12 2015 +0100

    PEP8 fixes

commit 27d6838e190ca331b656c5747f643c5164d1302f
Author: Thomas Robitaille <thomas.robitaille@gmail.com>
Date:   Wed Aug 26 09:45:45 2015 +0100

@astrofrog
Copy link
Member

@stscieisenhamer - actually this all looks fine! You can now force push to here:

git push -f origin extractor_drag

You should definitely not run git pull at the end - this would try and merge the github version into your branch which wouldn't work. So basically what you have above is fine, and just finish up with a git push -f ....

@stscieisenhamer
Copy link
Contributor Author

Done! So that would be my failing. BTW I did try the git pull --rebase, which also seems to work, but all the commits from the point of branch divergence get thrown in there. Which, could be edited down. So, it seems at this point its a matter of preference. shrug Again apologies for the git ignorance.

@astrofrog
Copy link
Member

@stscieisenhamer - thanks!

I like the ability to press control and drag the selection, but I don't think the spectrum needs updating while it's being drawn because of the performance considerations. Maybe you could direct the callback to an intermediate function that only then calls _update_profile if the spectrum has already been drawn once?

@stscieisenhamer
Copy link
Contributor Author

Just to clarify: You mean by "drawn" in the sense of the creation of the region. Only do the updating when the region exists and is then being dragged. Correct? If so, this makes sense.

@astrofrog
Copy link
Member

@stscieisenhamer - yes, that is correct.

@astrofrog
Copy link
Member

@stscieisenhamer - looks good, thanks! I'll add a changelog entry.

astrofrog added a commit that referenced this pull request Sep 9, 2015
Added dragging for dynamic spectrum updating.
@astrofrog astrofrog merged commit f5ecabd into glue-viz:master Sep 9, 2015
astrofrog added a commit that referenced this pull request Sep 9, 2015
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.

3 participants