radio-astro-tools / radio-beam

A simple toolkit for reading and manipulating beams from astrophysical radio spectral data cubes.

https://radio-beam.readthedocs.io/en/latest/

BSD 3-Clause "New" or "Revised" License

26 stars 21 forks source link

Fix beam creation from an area #94

Closed smaret closed 3 years ago

smaret commented 3 years ago

Closes #90.

smaret commented 3 years ago

but maybe we should force area.to(u.deg**2) ? or rad = sqrt(area).to(u.deg) ?

The units of major and minor are not forced to u.deg when creating a beam from these parameters, so I don't think that we need to force area units neither. We may want to check wether area units are equivalent to u.deg**2 though.

e-koch commented 3 years ago

Agreed @smaret -- A check for an area equiv. unit makes sense. thanks!

e-koch commented 3 years ago

The major/minor do get checked for the unit type. This check would just be for an easier to understand error message

smaret commented 3 years ago

The major/minor do get checked for the unit type

Here you mean ? https://github.com/radio-astro-tools/radio-beam/blob/ee8ed32ce3376a136ef1048746a9ab55dfd8cbb3/radio_beam/beam.py#L73-L77

smaret commented 3 years ago

The test is weird. If one provides major with an invalid unit, it is multiplied by u.deg and a warning message saying that degrees are assumed is displayed. But later on a UnitConversionError is thrown.

smaret commented 3 years ago

Perhaps the last line should read:

major = major.value * u.deg

But that's a separate issue.

codecov-commenter commented 3 years ago

Codecov Report

Merging #94 (c5c0691) into master (ee8ed32) will increase coverage by 0.48%. The diff coverage is 100.00%.

:exclamation: Current head c5c0691 differs from pull request most recent head 46dab3d. Consider uploading reports for the commit 46dab3d to get more accurate results

@@            Coverage Diff             @@
##           master      #94      +/-   ##
==========================================
+ Coverage   85.35%   85.83%   +0.48%     
==========================================
  Files          12       12              
  Lines        1304     1313       +9     
==========================================
+ Hits         1113     1127      +14     
+ Misses        191      186       -5

Impacted Files	Coverage Δ
radio_beam/beam.py	`84.70% <100.00%> (+2.09%)`	:arrow_up:
radio_beam/tests/test_beam.py	`96.38% <100.00%> (+0.11%)`	:arrow_up:

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 ee8ed32...46dab3d. Read the comment docs.

e-koch commented 3 years ago

Perhaps the last line should read:
major = major.value * u.deg
But that's a separate issue.

Agreed. I'll make a new issue. I think it was to allow non Quantities, but we should really remove it for consistency.