First Release of PSF Notebook: hst_point_spread_function

mrevalski commented 1 month ago

This notebook checklist has been made available to us by the Notebooks For All team. Its purpose is to serve as a guide for both the notebook author and the technical reviewer highlighting critical aspects to consider when striving to develop an accessible and effective notebook.

The First Cell

[x] The title of the notebook in a first-level heading (eg. <h1> or # in markdown).
[x] A brief description of the notebook.
[x] A table of contents in an ordered list (1., 2., etc. in Markdown).
[x] The author(s) and affiliation(s) (if relevant).
[x] The date first published.
[x] The date last edited (if relevant).
[x] A link to the notebook's source(s) (if relevant).

The Rest of the Cells

[x] There is only one H1 (# in Markdown) used in the notebook.
[x] The notebook uses other heading tags in order (meaning it does not skip numbers).

Text

[x] All link text is descriptive. It tells users where they will be taken if they open the link.
[x] All acronyms are defined at least the first time they are used.
[x] Field-specific/specialized terms are used when needed, but not excessively.

Code

[x] Code sections are introduced and explained before they appear in the notebook. This can be fulfilled with a heading in a prior Markdown cell, a sentence preceding it, or a code comment in the code section.
[x] Code has explanatory comments (if relevant). This is most important for long sections of code.
[x] If the author has control over the syntax highlighting theme in the notebook, that theme has enough color contrast to be legible.
[x] Code and code explanations focus on one task at a time. Unless comparison is the point of the notebook, only one method for completing the task is described at a time.

Images

[ ] All images (jpg, png, svgs) have an image description. This could be
- [ ] Alt text (an alt property)
- [ ] Empty alt text for decorative images/images meant to be skipped (an alt attribute with no value)
- [ ] Captions
- [x] If no other options will work, the image is decribed in surrounding paragraphs.
[ ] Any text present in images exists in a text form outside of the image (this can be alt text, captions, or surrounding text.)

Visualizations

[x] All visualizations have an image description. Review the previous section, Images, for more information on how to add it.
[ ] Visualization descriptions include
- [ ] The type of visualization (like bar chart, scatter plot, etc.)
- [ ] Title
- [ ] Axis labels and range
- [ ] Key or legend
- [ ] An explanation of the visualization's significance to the notebook (like the trend, an outlier in the data, what the author learned from it, etc.)
[ ] All visualizations and their parts have enough color contrast (color contrast checker) to be legible. Remember that transparent colors have lower contrast than their opaque versions.
[ ] All visualizations convey information with more visual cues than color coding. Use text labels, patterns, or icons alongside color to achieve this.
[ ] All visualizations have an additional way for notebook readers to access the information. Linking to the original data, including a table of the data in the same notebook, or sonifying the plot are all options.

review-notebook-app[bot] commented 1 month ago

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

mrevalski commented 1 month ago

This draft pull request was created to run the DMD CI checks. Specifically, to understand if the FORTRAN executable call succeeds.

mrevalski commented 1 month ago

Hi @FDauphin, I've corrected all of the PEP8 issues for this notebook and it is ready for your science review before Hatice completes the full technical review. @haticekaratay, I'm encountering an error with the execution checks, which state that a file is not found, but that file is produced in the previous cell via a FORTRAN executable in the same directory.

My initial thought is that I need to explicitly include the FORTRAN dependency in my requirements file, or perhaps it's not included in the CI system machinery, or simply that the architectures are different so the executable fails to run. As your time allows, do you have suggestions for how I should try to resolve this error? Thank you for your time and help!

mrevalski commented 1 month ago

Hi @FDauphin, I've corrected all of the PEP8 issues for this notebook and it is ready for your science review before Hatice completes the full technical review. @haticekaratay, I'm encountering an error with the execution checks, which state that a file is not found, but that file is produced in the previous cell via a FORTRAN executable in the same directory.

My initial thought is that I need to explicitly include the FORTRAN dependency in my requirements file, or perhaps it's not included in the CI system machinery, or simply that the architectures are different so the executable fails to run. As your time allows, do you have suggestions for how I should try to resolve this error? Thank you for your time and help!

Hi everyone, Fred Dauphin was able to help me solve this issue. The solution was two fold. First, we added a pre-requirements.sh file with "conda install --yes -c conda-forge gfortran" to ensure FORTRAN was installed in the CI environment. Next, we removed the pre-compiled executable, provided the FORTRAN code in the form of a *.F source code file, and then had the notebook compile it with a BASH command. The full set of CI checks now pass!

mrevalski commented 1 month ago

Hi @FDauphin, thank you for the very helpful review! Your revisions have greatly improved the clarity and coding cleanliness of the notebook. I've incorporated the vast majority of your comments exactly as suggested, and replied to a small number above for discussion. I think the main outstanding items are the following:

Should we move all of the auxiliary files such as the workflow diagram, catalog and FORTRAN files, to a "docs" folder?
Is it okay to use if var is True for Boolean statements, or should I abbreviate this to simply if var for conciseness?
It would be beneficial to add a comparison of all the methods, but this is tricky as I don't want to give the impression that any one method is "better" as it depends on the dataset and science goals. Similarly the drizzle case is separate. Thoughts?

In keeping @haticekaratay in the loop, once Fred has reviewed my changes and we iterate on the last few items listed above, we'll convert this from a draft to a full pull request for your review. We hope to give you a full working week to review and revise this with us, with the goal of pulling it into the main branch before June 7 so we can link to the new notebook in the Space Telescope Announcement Newsletter (STAN). Please let me know if this timeline will work with your existing commitments.

Have a great day!

mrevalski commented 1 month ago

Thanks for addressing each comment, everything is coming together nicely! All of your functions are clearly explained. There's some small documentation changes and f-string suggestions. I also replied to a few comments, including:

psf_utilites: return for save_figure()

psf_utilities: optimizing create_mask()

notebook: making docs folder

notebook: make_model_star_image in wfc3tools

notebook: WFC/UVIS for ACS/WFC3

notebook: # was flt

notebook: numpy citation

In addition, I think adding more descriptive filenames to your figures will be helpful. It was hard to remember what "fig6" was, and it's nice seeing all the titles if several of them are open in Adobe.

After these get addressed, we should be good for a DMD review 🙌🏾

Hi @FDauphin, thank you for the detailed and thoughtful review! I have incorporated all of your comments to the best of my ability, and the notebook is ready for your final review before we pass it to Hatice. Please let me know if you see any issues. In regards to image_array I updated this to image_list and fixed the docstring, as otherwise it causes a crash.

As a final question, do you think the descriptive list of imports is overly verbose? For example, I could remove the third level of bullets under photutils.aperture and photutils.psf and simply link to the main packages. It doesn't hurt since the information is already there, but it may help to make the notebook more concise?

I think we have a nice list of features that would be excellent to add in "v2.0", which I'll tackle in the future after discussions with Sylvia and the PSF team. Please let me know when you've completed your review and I'll convert this from a draft to a formal pull request. Thank you again, I really appreciate your time and help! 🙂

mrevalski commented 1 month ago

Hey @mrevalski thanks for all your hard work! Replying to your comments here:

A possible bug on copying pert output files.

If you want to remove the third bulleted layer for photutils (aperture and psf), that's fine with me. The original problem was one of them went out to the third layer and the other didn't. As long as it's consistent, either to two or three layers is fine.

For conclusions section, it's mostly for consistency across WFC3 notebooks, as each one has a small conclusions section. There you can quickly bullet point the various methods performed and learned.

I'm confident these will be your final commits. Thanks for being patient!

Hi @FDauphin, thank you for the quick response and excellent feedback. I've corrected the catalog copying error, and decided to retain the third level of bullets for now. If more packages are used in the future then we'll abbreviate this list. Great idea, I've added back in the Conclusions section, and created a brief summary of all the techniques, linked to each section, for the user.

Many thanks again! Please let me know if you have any remaining suggestions, or we're ready for DMD review. 🙂

haticekaratay commented 1 month ago

Your notebook is exceptionally well-crafted and comprehensive. The introduction is clear and informative, and the links to tutorials and additional resources are incredibly helpful. The way you've linked every imported package and the workflow is exemplary. This is one of the best notebooks I've read so far.

mrevalski commented 1 month ago

Your notebook is exceptionally well-crafted and comprehensive. The introduction is clear and informative, and the links to tutorials and additional resources are incredibly helpful. The way you've linked every imported package and the workflow is exemplary. This is one of the best notebooks I've read so far.

Hi @haticekaratay, thank you for the helpful review and very, very kind words! While I invested a lot of effort into this notebook, I owe much credit to @FDauphin and the WFC3 team for the helpful suggestions. I have implemented all of your changes, and also made some minor tweaks to improve the wording for a few descriptions in the markdown, and the notebook is now ready for your review. Please let me know if there are any additional polishing touches that are required before this can be merged!

spacetelescope / hst_notebooks