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

Add elliptical ROI button to imviz #706

Merged
merged 3 commits into from
Jul 6, 2021

Conversation

javerbukh
Copy link
Contributor

@javerbukh javerbukh commented Jun 30, 2021

Requires glue-viz/glue-jupyter#241
Elliptical ROI functionality added in glue-jupyter v0.7

Resolves #635

Screen Shot 2021-06-30 at 2 58 29 PM

Screenshot is slightly outdated, a better icon for elliptical ROI is being used.

@github-actions github-actions bot added the imviz label Jun 30, 2021
@pllim pllim added this to the Imviz 1.0 milestone Jun 30, 2021
Copy link
Contributor

@ibusko ibusko left a comment

Choose a reason for hiding this comment

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

The button creates a circular region; I couldn't figure out how to make it elliptical.

@codecov
Copy link

codecov bot commented Jul 2, 2021

Codecov Report

Merging #706 (3b27caf) into main (9ce4a91) will increase coverage by 0.64%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #706      +/-   ##
==========================================
+ Coverage   61.05%   61.70%   +0.64%     
==========================================
  Files          65       65              
  Lines        4157     4222      +65     
==========================================
+ Hits         2538     2605      +67     
+ Misses       1619     1617       -2     
Impacted Files Coverage Δ
jdaviz/configs/imviz/plugins/viewers.py 40.00% <ø> (-0.75%) ⬇️
jdaviz/configs/imviz/helper.py 98.71% <0.00%> (+3.16%) ⬆️

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 9ce4a91...3b27caf. Read the comment docs.

Copy link
Contributor

@ibusko ibusko left a comment

Choose a reason for hiding this comment

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

Works as expected.

@pllim
Copy link
Contributor

pllim commented Jul 2, 2021

As discovered by @rosteen , "get region" for this new shape does not work without glue-viz/glue-astronomy#32 . If this is merged before glue-astronomy with that patch can be released, we need to open a new issue as follow up.

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 see the same "lag in moving it" and "get region breakage" that @rosteen reported.

I will withhold approval until we know whether to wait for new glue-astronomy release or not.

Nice work otherwise!

@rosteen
Copy link
Collaborator

rosteen commented Jul 2, 2021

Not sure if this possibility was mentioned offline, but now that the required glue-astronomy PR was merged we could change that requirement pin to the dev version and create a follow-up issue here to update the pin to the newest version once an actual glue-astronomy release happens.

@pllim
Copy link
Contributor

pllim commented Jul 2, 2021

pin to the dev version

There was a precedent... I think that is acceptable.

@pllim
Copy link
Contributor

pllim commented Jul 2, 2021

Confirmed that "get region" is fixed with glue-astronomy dev version:

>>> regions = imviz.app.get_subsets_from_viewer('viewer-1')
>>> regions
{'Subset 1': <EllipsePixelRegion(PixCoord(x=1539.462646484375, y=1322.1376953125), width=626.53564453125, height=88.5322265625, angle=0.0 deg)>}

The lag still there.

@rosteen
Copy link
Collaborator

rosteen commented Jul 6, 2021

@javerbukh If you update the glue-astronomy dependence to the dev version I'll approve and merge this, and create a follow-up issue to update the dependence after the next glue-astronomy release.

@javerbukh
Copy link
Contributor Author

@rosteen Done, thank you!

@pllim
Copy link
Contributor

pllim commented Jul 6, 2021

FWIW, I opened #711 and #712 as follow up issues. I'll leave the merge for @rosteen . Thanks, everyone!

@rosteen rosteen merged commit 31181b1 into spacetelescope:main Jul 6, 2021
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.

JDAT-1485: Imviz region support: Add ellipse
4 participants