search

casangi / xradio

Xarray Radio Astronomy Data IO
Other
9 stars 5 forks source link

Fix issues in read_image when opening FITS #157

Closed Mark1626 closed 2 months ago

Mark1626 commented 2 months ago

Addresses part of #155

Summary

  1. Since doppler is expected, default set it to RADIO
  2. Move variable cunit out of call to _get_freq_values
  3. Add FREQ to _get_transpose_list option
  4. Address minor typos

Any comments on point 1?

Let me know if this is fine, I'll be happy to changes based on your suggestions

CLAassistant commented 2 months ago

CLA assistant check
All committers have signed the CLA.

dmehring commented 2 months ago

Thanks for the code changes.

I will start by saying that I am no FITS expert, and that we really need one to ensure the FITS reader is correct and fully supports everything users expect it to support; FITS is one of those "standards" that has evolved to support a lot of metadata, some often used, some not. In my mind, FITS support is low priority in xradio at the moment because CASA and zarr image support has higher priority. If one has a FITS image, they can always import that info CASA and read the CASA image with xradio. In my mind, CASA image metadata is much better defined than FITS.

From what I can tell, these changes look reasonable. It would be very helpful if you could add tests to the image unit test suite that cover the code you changed. That way, it will be less likely your changes will be reverted or broken in the future. Thanks again.