search

spacetelescope / poppy

Physical Optics Propagation in Python
https://poppy-optics.readthedocs.io
BSD 3-Clause "New" or "Revised" License
219 stars 72 forks source link

Documentation of FITS header keywords #202

Closed mperrin closed 6 years ago

mperrin commented 6 years ago

Issue by douglase Friday Feb 10, 2017 at 17:12 GMT Originally opened as https://github.com/mperrin/poppy/issues/202

I'd like to add a list of header keywords used by POPPY to the documentation so I can point people to it (instead of duplicating it in an ICD).

e.g.:

I see two ways of implementing this, one would be to add detail to each of the functions that generate a FITS file (calc_psf() and as_fits()) and possibly those that read in a FITS (though this is documented in extending.rst or just create a master list in the extending.rst file.

I am leaning toward the latter, since it makes things more extensible and provides 'one-stop' shopping for keywords. Any objections?

mperrin commented 6 years ago

Comment by mperrin Friday Feb 10, 2017 at 18:51 GMT

I wil never object to anyone making the documentation better. :-) Go for it.

I agree that doing it the latter way is quicker and easier. A minor suggestion is I wouldn't put it in the extending.rst file though, I think it should be its own page in the documentation. (Lately it seems like best practices for documentation are to have more pages which are each self-contained to a single topic, rather than trying to put too many unrelated things onto a single documentation page.)

mperrin commented 6 years ago

Comment by douglase Friday Feb 10, 2017 at 19:51 GMT

@mperrin, is it intentional that the output pixel scale keyword is PIXELSCL (https://github.com/mperrin/poppy/blob/master/poppy/poppy_core.py#L285) and OPD files use PIXSCALE? (https://github.com/mperrin/poppy/blob/master/poppy/poppy_core.py#L2744)

mperrin commented 6 years ago

Comment by josePhoenix Friday Feb 10, 2017 at 19:53 GMT

I had a sinking feeling that I was the last to touch that code, but it looks like I didn't change the keyword when I refactored it... https://github.com/mperrin/poppy/commit/a131f98dc7935e991ef47d7ab7048fe80596fb99

mperrin commented 6 years ago

Comment by mperrin Friday Feb 10, 2017 at 20:00 GMT

Hmm, those should probably be made consistent, yeah. And I think we maybe need another keyword like PIXUNIT to say if it's arcsec/pixel or meters/pixel, right?

They may have originally been different because PIXELSCL is in arcsec/pix for image planes but PIXSCALE is in meters/pix for pupil planes. But I may also just not have thought about this carefully enough when originally setting things up.

Let's also not forget about AnalyticOpticalElement.to_fits() which also makes FITS files in a maybe-not-consistent way. See #49.

mperrin commented 6 years ago

Comment by mperrin Monday Jul 10, 2017 at 23:08 GMT

@douglase, was your PR #205 sufficient to close this issue, or is there more you wanted to do for it?

mperrin commented 6 years ago

Comment by douglase Tuesday Jul 11, 2017 at 00:16 GMT

I'm content.