search

spacetelescope / webbpsf

James Webb Space Telescope PSF simulation tool
https://webbpsf.readthedocs.io
BSD 3-Clause "New" or "Revised" License
119 stars 63 forks source link

Add "units" as attribute to OPD class #300

Open kjbrooks opened 5 years ago

kjbrooks commented 5 years ago

Part of the solution to #230 (#232) added the BUNIT keyword to the OPD class, however, I think it is important to add this as an attribute of any OPD object itself. The thermal slew OPD code can, and probably will, be run such that no file is written out, but instead the user has an OPD object. What I am finding is that when combining with the requirements OPDs (which are in units of microns) there is no way to know that the OPD you made is in units of meters, unless you know.

I think adding a units attribute to the OPD class would solve this problem and then can be called directly in as_fits when adding the BUNIT keyword to the header. The only question here would be if it's better to add this to poppy's FITSOpticalElement class directly, instead of in webbpsf (if that is determined to be the case, I can open an issue in poppy instead)

mperrin commented 5 years ago

The OPD files used with webbpsf are in units of microns as files on disk, yes, but they get converted into meters automatically immediately upon being opened. This happens in poppy.FITSOpticalElement.__init__. By design we try to have all OPDs objects or arrays in memory always be in units of meters, for consistency & simplicity. So I think it is always the case in webbpsf that the results from the thermal slew OPD calculations can be added directly to any of the OPDs read from disk.

I'm not opposed to adding this as a new attribute. But I would like to hope that its value will always be in meters.

kjbrooks commented 5 years ago

That is a good point. However, isn't true that they only get converted to meters automatically if opened by poppy, not if you open them simply in with astropy.io.fits?

I guess where I am getting hung up, is that I used a function that you (@mperrin ) wrote to combine OPDs and it reads the requirements/predicted OPD fits files with astropy.io.fits which means that they do not get converted to meters. I can easily building into my own example code the necessary conversion, but it feels like I am only aware of this because I have had to work with code.

In trying to separate my own current issue, from the issue of adding units, I think that unless we can guarantee that users will be handling their OPD objects and files as we expect them too, more communication is better than less. This does seem to be something that would be better addressed in poppy, so this issue probably should be closed.