Docs improvements, including for matching PSFs to data

[mperrin]

Documentation improvements.

• Improve docs about detector_position and source_offset, and how they relate. Addresses setup_sim_to_match_data documentation improvement #738, and several recent user questions.
• Update psf_grid docs. Addresses Using PSF Grids - webbpsf.gridded_library.display_psf_grid(grid) does not provide correct labels #887.
• Great big bunch of docs improvements to the main "Using WebbPSF" page, including converting it from .rst to .ipynb format, ensuring all code examples are runnable, and expanding/improving doc text extensively.
• Switch sphinx style file and some conf.py options to match those used by the JWST pipeline readthedocs, improving the aesthetics a bit.

[mperrin]

FYI @rcooper295 and @m-samland, I've written some hopefully-improved docs about the detector_position and source_offset parameters for webbpsf and how they work. You can view the revised docs page here.

Your feedback would be much appreciated about whether the updated docs page is clearer enough, if there are any parts I could further explain, if you have any additional questions about this stuff, etc. Thanks so much!

[rcooper295]

The changes to the docs looks great, this did help clarify some remaining uncertainty I had about how to use the detector_position correctly. I did find one very minor typo: an extra "the" in this sentence: "This attribute sets the which pixel on the detector corresponds to the center pixel location of the subarray of pixels output by webbpsf."
Thanks for this!

[mperrin]

Thanks @rcooper295, much appreciated for that typo fix!

Right after that part of the docs I have one more paragraph I think I'd like to add, which addresses another detail about detector_position. Does the following text make sense, please? Feedback welcome whether this is clear or could be improved:

Note, for science images taken using a detector subarray instead of the full detector, after using setup_sim_to_match_data the detector_pixel coordinates will refer to the pixel coordinates in that subarray only, not the full detector. In other words, simply set detector_pixel to the desired (X,Y) coordinates within the specified science image, and it will work correctly for either full-frame or subarray science data.

[rcooper295]

I agree that's a good addition. After looking at it some more this afternoon I thought it might also be helpful to clarify that in order to match as closely as possible to the real position of data in a detector subarray (meaning one of the pre-defined readout subarrays) a combination of setting detector_position and x_offset/y_offset will usually be needed. I guess this could potentially be an added option for setup_sim_to_match_file in the future -- it could read the aperture info and offset from the file header and place it in that position?
Users might naively (like me) think that if they set the fov to the size of the subarray, and the detector_position matches the aperture reference pixel position (which at least for NIRISS/SUB80 is the first primary dither position, which is not at the array center), the position of the simulated psf with respect to the simulated subarray edges would match that of the data. But maybe that's overkill and most people would figure that out for themselves?
The sentence you have now in the "Adjusting the position of the simulated point source" section of the docs may potentially be misleading: "You may think of this as simulating a star that is precisely centered in that subarray of pixels on the detector." If I'm understanding correctly, this refers to an arbitrary subarray of pixels rather than one of the predefined subarrays of the detector, though it will likely overlap considerably with the real subarray.
Apologies if this is hard to parse -- I can provide a more an example using actual numbers if that would be helpful.

[mperrin]

Thanks Rachel, that's useful feedback and I will continue to tweak the text!

[mperrin]

I think I'll switch to "subregion" or "image cutout" to refer to the subset of pixels being modeled by webbpsf, and use "subarray" strictly to refer to predefined named detector subarrays as used on the hardware. That may clarify better. Good catch!

[pep8speaks]

Hello @mperrin, Thank you for updating !

• In the file docs/conf.py:

Line 124:1: E265 block comment should start with '# '
Line 128:1: E265 block comment should start with '# '
Line 153:1: E265 block comment should start with '# '

Comment last updated at 2024-09-11 18:26:58 UTC

[mperrin]

FYI @obi-wan76 @BradleySappington my big block 'o docs improvements is ready for review. Significant update and refresh to the main "usage" page, including converting from RST to Jupyter notebook with fully runnable code, and generally enhancing and updating the text there. Also a lot of update and expansion to the matching data page as well, and some lesser edits elsewhere.

This is a big enough block of changes that it may be a bit tricky to review, plus the diff tools for notebooks aren't as easy to use as for plain text. I'll leave it to your discretion how closely or not to go over this. Maybe Marcio could look at it for text and content, and Brad for whether it still builds correctly to HTML for readthedocs? But, your call, and I'll be fine with however you prefer. Thanks!

[obi-wan76]

Thanks! I'll take a look at it.

[BradleySappington]

Everying builds in HTML properly with a few warnings. Probably makes sense to fix them with this PR:

webbpsf/webbpsf/webbpsf/webbpsf_core.py:docstring of webbpsf.webbpsf_core.SpaceTelescopeInstrument:14: WARNING: undefined label: 'spacetelescopeinstrument.options'

webbpsf/webbpsf/docs/index.rst:27: WARNING: undefined label: 'using_api'

webbpsf/webbpsf/docs/references.rst:29: WARNING: undefined label: 'normalization'

webbpsf/webbpsf/docs/release.rst:88: WARNING: undefined label: 'install-with-conda'

webbpsf/webbpsf/docs/relnotes.rst:355: WARNING: undefined label: 'wfirst_wfi'

webbpsf/webbpsf/docs/roman.rst:28: WARNING: undefined label: 'using_api'

webbpsf/webbpsf/docs/src/poppy/docs/fft_optimization.rst:6: WARNING: undefined label: '_performance_and_parallelization'

[obi-wan76]

This figure probably should be updated. The current filter read filter 200W, it should be F212N and in the new Photutils ePSF mode, there is also some ePSF texts that appears in the figure in the new version. You can commit the figures in this review to this particular document. See example

[obi-wan76]

This looks good and it worked as intended. Given that your are showing the difference between observations and simulations we could also add a reference/link to the PSF subtraction notebook/doc, which shows a different approach to a similar problem.

As a side note, I like the plot_data_sim_comparison function. During my review, locally, I added the phase_cross_correlation shift to the simulation in the same way you did it in nrc_ta_image_comparison. By shifting the simulated PSF the difference looks better (?) but the chi2 is bigger. In any case, I thought this could have improved the comparison by mimicking the TA comparison but it seems that it did not work as intended.

[mperrin]

@obi-wan76 I addressed your comments about needing to update psf_grids.rst by entirely converting it to psf_grids.ipynb so it's now directly runnable, and all figures are thus directly consistent with what the code outputs now.

@BradleySappington I fixed those build warnings, but it's a little hacky FYI. As far as I can tell, nbsphinx docs made using Jupyter notebooks aren't directly able to use the named cross reference links like Sphinx RST files would (hence those warnings). I therefore replaced those links with HTML links to the URLs of HTML files that are produced when the docs are built. This still leads to some build warnings, about File Not Found for various of the HTML files, I guess due to hyperlinks to HTML files not yet built at the time those warnings are emitted. But it all works once the files are all compiled.

I think this is ready for re-review and/or merge. Thanks for the helpful feedback.

[BradleySappington]

Thanks @mperrin, if that is all functioning then I'm good with this (even with warnings). I'll leave it to @obi-wan76 to finish his review/merge.

[obi-wan76]

@obi-wan76 I addressed your comments about needing to update psf_grids.rst by entirely converting it to psf_grids.ipynb so it's now directly runnable, and all figures are thus directly consistent with what the code outputs now.

@BradleySappington I fixed those build warnings, but it's a little hacky FYI. As far as I can tell, nbsphinx docs made using Jupyter notebooks aren't directly able to use the named cross reference links like Sphinx RST files would (hence those warnings). I therefore replaced those links with HTML links to the URLs of HTML files that are produced when the docs are built. This still leads to some build warnings, about File Not Found for various of the HTML files, I guess due to hyperlinks to HTML files not yet built at the time those warnings are emitted. But it all works once the files are all compiled.

I think this is ready for re-review and/or merge. Thanks for the helpful feedback.

@mperrin I don't see the file psf_grids.ipynb, is it in this PR?

[mperrin]

@obi-wan76 Whoops, my mistake! I added that file just now.

[obi-wan76]

Looks good. Thanks!