Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update STIPS for version 2.0 release #163

Merged
merged 54 commits into from
Sep 12, 2022

Conversation

york-stsci
Copy link
Collaborator

@york-stsci york-stsci commented Jul 6, 2022

This is a general cleanup PR to get STIPS ready for the Version 2.0 release.

Closes #162
Closes #161
Closes #160
Closes #159
Closes #158
Closes #152
Closes #148
Closes #147
Closes #146

@york-stsci york-stsci added this to the 2.0.0 milestone Jul 6, 2022
@york-stsci york-stsci self-assigned this Jul 6, 2022
@york-stsci
Copy link
Collaborator Author

Removed issues 144 and 145 because they are lower priority compared to the work they would require.

docs/basic_tutorial.rst Show resolved Hide resolved
docs/examples.rst Outdated Show resolved Hide resolved
notebooks/STIPS Tutorial.ipynb Outdated Show resolved Hide resolved
docs/bugs.rst Outdated Show resolved Hide resolved
docs/installation.rst Show resolved Hide resolved
environment.yml Outdated Show resolved Hide resolved
stips/astro_image/tests/test_AstroImage.py Outdated Show resolved Hide resolved
stips/tests/test_roman.py Outdated Show resolved Hide resolved
stips/instruments/instrument.py Outdated Show resolved Hide resolved
stips/utilities/makePSF.py Show resolved Hide resolved
Some modifications to the astro_image WebbPSF PSF generation produced incorrect PSFs. I've reverted some of the changes to the original format, while also removing the "oversampling" parameter, which is no longer used.
Copy link
Collaborator

@ojustino ojustino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @york-stsci, I did an initial review of the documentation and recommend reacting to the comments in this order:

  1. Most of my comments are actually suggestions that you can accept through the GitHub interface. I rendered the documentation locally to confirm that they work as expected, so most should be uncontroversial. Make sure to accept the ones you agree with as a batch so the changes go into a single commit instead of one commit per suggestion.
  2. Next, pull the new commit into your local branch.
  3. After that, there are a couple of comments where I've asked you to copy and paste code blocks into certain files. If you agree with the changes, fix those in this step.
  4. In basic_tutorial.rst, index.rst, the first section of installation.rst, and using_stips/psf_grid.rst, many lines of text are very long and should be split into 80-100 characters each. The exact number isn't important here, but doing this will make it easier to track differences and make suggestions in these files. Please only do this after you make the decisions on whether to accept my suggestions and pull the new commit to your local branch. I will go back and make my remaining suggestions to those files once that's done.

Other points:

  • It looks like bulleted lists don't work with stsci_rtd_theme unless they are on an inner level. jwst uses the same theme and outer bullets don't appear in their documentation, either. We could switch to another theme in docs/conf.py like sphinx_rtd_theme if that's an issue, but we'd need to make sure everything looks as expected afterward.
  • If you didn't know, you can render the documentation locally to make sure the changes you make look the way you intend. Here are the steps in the terminal:
    1. pip install sphinx
    2. Make a new directory inside docs to hold the rendered HTML files.
    3. sphinx-autobuild -a PATH-TO-DOCS PATH-TO-NEW-DIRECTORY
    4. Visit each of the new directory's HTML file paths in a browser to view the rendered webpages.
    5. Repeat from sphinx-autobuild down any time you save changes to an RST file that you'd like to see rendered.

docs/basic_tutorial.rst Outdated Show resolved Hide resolved
docs/basic_tutorial.rst Outdated Show resolved Hide resolved
docs/basic_tutorial.rst Outdated Show resolved Hide resolved
docs/basic_tutorial.rst Outdated Show resolved Hide resolved
docs/basic_tutorial.rst Outdated Show resolved Hide resolved
docs/using_stips/catalogue_formats.rst Outdated Show resolved Hide resolved
docs/using_stips/config_file.rst Outdated Show resolved Hide resolved
docs/using_stips/config_file.rst Outdated Show resolved Hide resolved
docs/using_stips/psf_grid.rst Outdated Show resolved Hide resolved
docs/using_stips/psf_grid.rst Outdated Show resolved Hide resolved
Copy link
Collaborator

@ojustino ojustino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you got all of the copy/paste suggestions from the first review except this one.

Once again, most of the new suggestions are small changes. I want to standardize the appearance of catalog types in using_stips/catalogue_formats.rst and of package names in insatllation.rst, but there are so many occurrences that I think it'll be easier to make the changes myself and, if I can, push them directly to your branch once you finish this batch of edits.

I think the problem with bulleted lists is indeed due to stsci_rtd_theme; sphinx_rtd_theme had to make some changes themselves last year to fix the same issue. For now, I propose replacing all * characters used to make outer lists with \• to manually render a bullet character. If you're OK with that, I will add it to my follow-up commit, too.

docs/basic_tutorial.rst Outdated Show resolved Hide resolved
docs/basic_tutorial.rst Outdated Show resolved Hide resolved
docs/basic_tutorial.rst Outdated Show resolved Hide resolved
docs/basic_tutorial.rst Outdated Show resolved Hide resolved
docs/examples.rst Outdated Show resolved Hide resolved
docs/using_stips/catalogue_formats.rst Outdated Show resolved Hide resolved
docs/bugs.rst Show resolved Hide resolved
docs/installation.rst Outdated Show resolved Hide resolved
docs/installation.rst Outdated Show resolved Hide resolved
docs/using_stips/catalogue_formats.rst Outdated Show resolved Hide resolved
docs/basic_tutorial.rst Outdated Show resolved Hide resolved
Copy link
Collaborator

@ojustino ojustino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new documentation commit needs some consistency edits to match past reviews. Once these and the PEP8 changes are in, I am available to issue an approval.

docs/index.rst Outdated Show resolved Hide resolved
docs/basic_tutorial.rst Outdated Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
Accepting documentation edits.

Co-authored-by: ojustino <[email protected]>
Copy link
Collaborator

@ojustino ojustino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the PEP8 edits helped improve readability, and since the tests still pass, it looks like they didn't introduce any unintended changes. Let's finish the release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment