search

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

Export `roi.theta` in 'astropy-regions' translator and run passing tests #73

Closed dhomeier closed 1 year ago

dhomeier commented 1 year ago

Description

As of 1.5.0 glue supports rotation angles in ROIs, breaking the NotImplementedError test. This is updating the test and exporting the rotation angle theta, if set, to Astropy regions created from an ROI. Also removed the pytest.raises for the no longer failing logical operations in test_invalid_combos.

codecov[bot] commented 1 year ago

Codecov Report

Merging #73 (8047414) into main (9996696) will increase coverage by 0.67%. The diff coverage is 94.11%.

@@            Coverage Diff             @@
##             main      #73      +/-   ##
==========================================
+ Coverage   96.95%   97.63%   +0.67%     
==========================================
  Files          15       15              
  Lines        1182     1182              
==========================================
+ Hits         1146     1154       +8     
+ Misses         36       28       -8
Impacted Files Coverage Δ
glue_astronomy/translators/tests/test_regions.py 99.30% <93.54%> (+2.76%) :arrow_up:
glue_astronomy/translators/regions.py 96.33% <100.00%> (+0.06%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

pllim commented 1 year ago

Oh, I just saw this. Will this completely fix https://github.com/glue-viz/glue-astronomy/issues/74 ? Any reason why this isn't merged yet?

dhomeier commented 1 year ago

Just wasn't time for a full review yet, and I forgot that programmatic support for rotation angles had already been introduced in regions v0.4 or so. So with test_ellipse_roi passing I think this should indeed fix your case.

pllim commented 1 year ago

And a small codestyle fix is needed:

glue_astronomy/translators/tests/test_regions.py:393:101: E501 line too long (106 > 100 characters)

dhomeier commented 1 year ago

Thanks for the review @pllim!

pllim commented 1 year ago

Thank YOU! Please let me know when this feature is released. 😸

pllim commented 1 year ago

@dhomeier or @astrofrog , any chance a release can happen soon with this patch? Thanks!

dhomeier commented 1 year ago

Initially I was hoping to get #72 in as well, but I think that still needs some review. So probably do a 0.4.1 release first.

pllim commented 1 year ago

If #72 can get in within a week, then we can probably wait.