USNavalResearchLaboratory / eispac

Read the Docs
https://eispac.readthedocs.io/en/latest/
MIT License
20 stars 6 forks source link

Use astropy.units #14

Open wtbarnes opened 2 years ago

wtbarnes commented 2 years ago

There are several places throughout the package where using units would be extremely helpful to both the user and in reducing the number of validation checks needed, e.g. ccd_offset, rot_xy. I think it would be a good idea to impose astropy units wherever possible. This would also help to increase interoperability with sunpy and the sunpy affiliated package ecosystem.

For a formal description of what this looks like in sunpy, see SEP-0003

PaulJWright commented 1 year ago

This would certainly help clear up confusion in ccd_offset, see #57. I am not going to ask this for the JOSS review, but I agree with @wtbarnes here.

MJWeberg commented 1 year ago

Thanks for the reminder! Implementing units for these functions is on our todo list, but it is fairly low-priority since these functions are used internally and should rarely, if ever, be called by users directly (additionally, ccd_offset is one of the only bits of code we ported directly over from IDL, so we want to make sure to maintain consistency if possible).

wtbarnes commented 1 year ago

Fair point about the internal use of those functions. I think I had just listed those as examples. Are there other user-facing functions that would benefit from taking units as inputs and giving units as outputs?

One place where this is already done is the EISMap class properties, e.g. the wavelength property.