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.

Sign up for GitHub

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

Jump to bottom

Added translator for CircleAnnulusPixelRegion #90

Merged
merged 4 commits into from
Apr 26, 2023
Merged

Added translator for CircleAnnulusPixelRegion #90

merged 4 commits into from
Apr 26, 2023

Conversation

pllim
Copy link
Contributor

@pllim pllim commented Apr 24, 2023 •
edited
Loading

This adds a one-way translator for CircleAnnulusPixelRegion.

Fix #49

@pllim pllim mentioned this pull request Apr 24, 2023
Closed
9 tasks
@codecov
Copy link

codecov bot commented Apr 24, 2023 •
edited
Loading

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.03 🎉

Comparison is base (478451f) 97.34% compared to head (6ca48f6) 97.38%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main      #90      +/-   ##
==========================================
+ Coverage   97.34%   97.38%   +0.03%     
==========================================
  Files          18       18              
  Lines        1355     1375      +20     
==========================================
+ Hits         1319     1339      +20     
  Misses         36       36
Impacted Files Coverage Δ
glue_astronomy/translators/regions.py 96.66% <100.00%> (+0.33%) ⬆️
glue_astronomy/translators/tests/test_regions.py 99.32% <100.00%> (+0.02%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@pllim
Copy link
Contributor Author

pllim commented Apr 24, 2023

I think Windows failure is transient download problem.

@dhomeier dhomeier added the enhancement New feature or request label Apr 25, 2023
@dhomeier
Copy link
Contributor

Just so I understand, the takes an accurately defined CircleAnnulusPixelRegion (with matching centres) as input, not the case of an imperfectly drawn one discussed in #49?

@pllim
Copy link
Contributor Author

pllim commented Apr 25, 2023

Yeah, I decided there is no point in guessing at an imperfect drawing. I am thinking of how to make this useful over at spacetelescope/jdaviz#2164

@dhomeier
Copy link
Contributor

I think Windows failure is transient download problem.

Yep, worked on re-run.

@pllim
Copy link
Contributor Author

pllim commented Apr 25, 2023

Thanks for the reviews! I believe I have addressed all the comments and added a test.

pllim
pllim commented Apr 25, 2023
View reviewed changes
glue_astronomy/translators/regions.py

# Put this here because there is nowhere else to put it.
# https://github.com/glue-viz/glue/issues/2390
def _annulus_to_subset_state(reg, data):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

xref glue-viz/glue#2390

pllim added a commit to pllim/jdaviz that referenced this pull request Apr 25, 2023
@pllim
POC: Annulus support in Imviz
9ea914e 
that needs glue-viz/glue-astronomy#90
dhomeier
dhomeier approved these changes Apr 26, 2023
View reviewed changes
Copy link
Contributor

@dhomeier dhomeier left a comment

Choose a reason for hiding this comment

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

Thanks for the fix and the tests cleanup! Should this rather go in as bugfix?

@pllim
Copy link
Contributor Author

pllim commented Apr 26, 2023

It is technically a new feature.

@dhomeier dhomeier changed the title Add translator for CircleAnnulusPixelRegion Add translator for CircleAnnulusPixelRegion Apr 26, 2023
@dhomeier dhomeier merged commit 336e2cc into glue-viz:main Apr 26, 2023
@dhomeier dhomeier changed the title Add translator for CircleAnnulusPixelRegion Added translator for CircleAnnulusPixelRegion Apr 26, 2023
@dhomeier dhomeier changed the title Added translator for CircleAnnulusPixelRegion Added translator for CircleAnnulusPixelRegion Apr 26, 2023
@pllim pllim deleted the annulus-translator branch April 26, 2023 15:50
@pllim
Copy link
Contributor Author

pllim commented Apr 26, 2023

Thanks!

This was referenced Apr 26, 2023
Merged
Merged
@pllim
Copy link
Contributor Author

pllim commented May 10, 2023

Is this released yet?

@Jdaviz-Triage-Bot Jdaviz-Triage-Bot mentioned this pull request May 10, 2023
Closed
This was referenced May 11, 2023
Merged
Merged
@pllim pllim mentioned this pull request May 12, 2023
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@astrofrog astrofrog astrofrog left review comments

@dhomeier dhomeier dhomeier approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Annulus region support
3 participants
@pllim @dhomeier @astrofrog