search

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

Swap major and minor axes if they're wrong #77

Closed keflavich closed 5 years ago

keflavich commented 5 years ago

if minor > major, they should be swapped (on reading)

coveralls commented 5 years ago

Coverage Status

Coverage increased (+0.1%) to 78.417% when pulling b209b5f9b8765bb5dfd8486ec0d18e57c5ae07af on keflavich:swapped_major_minor into 4a32182dab71faeed7285cef019dafac88885f28 on radio-astro-tools:master.

keflavich commented 5 years ago

If the user mis-specifies major and minor, they'll be swapped. Is there any risk that this causes trouble for users because they flipped the definition of the PA too? Or is the warning enough?

e-koch commented 5 years ago

I think we should adjust PA by 180 deg whenever this occurs as a default, and then have the warning state that the PA is adjusted. That way we're consistently transforming from the original inputs.

keflavich commented 5 years ago

you mean 90 deg?

keflavich commented 5 years ago

hm, maybe we should just raise an exception then..... I'm not convinced there's a clear default in this case.

e-koch commented 5 years ago

I definitely mean 90 deg.

e-koch commented 5 years ago

The exception seems reasonable to me. I'm imagining the case where a few highly elongated beams (that maybe you would mask out, anyways, but) end up giving weird convolution products if the PA is off by 90 deg.

e-koch commented 5 years ago

Otherwise, LGTM.