glue-viz / glue-astronomy

Plugin to add astronomy-specific functionality to glue
https://glue-astronomy.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
11 stars 12 forks source link

Support new CircularAnnulusROI #92

Closed pllim closed 1 year ago

pllim commented 1 year ago

Support new CircularAnnulusROI from glue-viz/glue#2403 (not yet merged when this PR is opened, so dev tests might fail, so we need to rerun that when it is merged upstream) by building on https://github.com/glue-viz/glue-astronomy/pull/90 .

Motivation: Instead of exposting the outer and inner circles separately in Jdaviz, we can now expose the proper annulus parameters by using this new ROI.

🐱

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 60.00% and project coverage change: -0.68 :warning:

Comparison is base (dab988a) 97.38% compared to head (8ce9ecc) 96.71%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #92 +/- ## ========================================== - Coverage 97.38% 96.71% -0.68% ========================================== Files 18 18 Lines 1379 1399 +20 ========================================== + Hits 1343 1353 +10 - Misses 36 46 +10 ``` | [Impacted Files](https://app.codecov.io/gh/glue-viz/glue-astronomy/pull/92?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glue-viz) | Coverage Δ | | |---|---|---| | [glue\_astronomy/translators/tests/test\_regions.py](https://app.codecov.io/gh/glue-viz/glue-astronomy/pull/92?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glue-viz#diff-Z2x1ZV9hc3Ryb25vbXkvdHJhbnNsYXRvcnMvdGVzdHMvdGVzdF9yZWdpb25zLnB5) | `98.66% <50.00%> (-0.67%)` | :arrow_down: | | [glue\_astronomy/translators/regions.py](https://app.codecov.io/gh/glue-viz/glue-astronomy/pull/92?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glue-viz#diff-Z2x1ZV9hc3Ryb25vbXkvdHJhbnNsYXRvcnMvcmVnaW9ucy5weQ==) | `91.48% <61.90%> (-5.29%)` | :arrow_down: |

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

pllim commented 1 year ago

The patch coverage will be low because for a given glue version, it will go a specific route, and not the other one.

pllim commented 1 year ago

And you will see import error for dev tests until https://github.com/glue-viz/glue/pull/2403 is merged.

pllim commented 1 year ago

A maintainer can please rerun the failed jobs. They should pass now, hopefully.

pllim commented 1 year ago

Thanks for the merge! What is your timeline for release?

dhomeier commented 1 year ago

The glue side is released in 1.11.0, right?

pllim commented 1 year ago

@dhomeier , I don't know as https://github.com/glue-viz/glue/pull/2403 is not milestoned.

dhomeier commented 1 year ago

It is https://github.com/glue-viz/glue/releases/tag/v1.11.0

pllim commented 1 year ago

@dhomeier , does this mean we need to bump glue pin here?

dhomeier commented 1 year ago

The tests passed with 1.10.0 as well https://github.com/glue-viz/glue-astronomy/actions/runs/4962038394/jobs/8949618535 (I am afraid we missed to clean up the GLUE_LT_1_10_1 here!); obviously the new translator would not be available then, but probably no need to enforce version >= 1.11.0.

pllim commented 1 year ago

missed to clean up

Ooops... Do you want me to PR?

dhomeier commented 1 year ago

Dunno, does this require a bugfix release? Functionally it should not make a difference, as there probably will be no glue-core 1.10.1... Could still fix it for the next regular release.

pllim commented 1 year ago

Oh, in that case, I don't think it needs to change? Just a matter of pedantic?