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

Undersampling docs #7589

Merged
merged 6 commits into from
May 26, 2023
Merged

Conversation

jotaylor
Copy link
Contributor

@jotaylor jotaylor commented May 16, 2023

This PR updates the undersampling_correction step description to fix some grammatical typos and improve the readability. There are no associated issues or Jira issues. However, one thing I didn't know how to do was link to a subsection of another page. So the words "charge migration" in the 3rd sentence should link to the Charge Migration subsection of the ramp_fitting step: https://jwst-pipeline.readthedocs.io/en/latest/jwst/saturation/description.html#charge-migration
Can someone please tell me how to do so?

Checklist for maintainers

  • added entry in CHANGES.rst within the relevant release section
  • updated or added relevant tests
  • updated relevant documentation
  • added relevant milestone
  • added relevant label(s)
  • ran regression tests, post a link to the Jenkins job below.
    How to run regression tests on a PR
  • Make sure the JIRA ticket is resolved properly

@jotaylor jotaylor requested a review from a team as a code owner May 16, 2023 21:31
@codecov
Copy link

codecov bot commented May 16, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (3c3edba) 77.48% compared to head (a60423e) 77.48%.

❗ Current head a60423e differs from pull request most recent head 09d3433. Consider uploading reports for the commit 09d3433 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #7589   +/-   ##
=======================================
  Coverage   77.48%   77.48%           
=======================================
  Files         456      456           
  Lines       36591    36591           
=======================================
  Hits        28354    28354           
  Misses       8237     8237           
Flag Coverage Δ *Carryforward flag
nightly 77.54% <ø> (ø) Carriedforward from b728a57

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files Coverage Δ
jwst/tweakreg/utils.py 93.33% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@hbushouse
Copy link
Collaborator

hbushouse commented May 19, 2023

@jotaylor Regarding the reference to charge migration (in the saturation docs), the most robust way is to create an explicit target in the saturation page and then reference it from the undersampling page. For example, at https://github.com/spacetelescope/jwst/blob/master/docs/jwst/saturation/description.rst?plain=1#L36 add a line containing

.. _charge_migration:

(yes, all the dots, underscores, and colon are required). Then in the undersampling page insert a reference/link to that target using

:ref:"charge_migration"

(again, all the punctuation is required). But NOTE that the double quotes around charge_migration should actually be a single back-tic, I just can't get github to render the back-tics properly, because they're also used for formatting! Just grep some of the other doc pages for the string ":ref:" and you'll see the formatting.

Copy link
Collaborator

@hbushouse hbushouse left a comment

Choose a reason for hiding this comment

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

Looks good. Just a few minor suggestions.

Oh, you also need to add an entry to the CHANGES.rst (change log) file at the top level of the repo. But an entry in the "documentation" subsection and follow existing entries for the proper formatting, including the PR number (7589).

docs/jwst/undersampling_correction/description.rst Outdated Show resolved Hide resolved
docs/jwst/undersampling_correction/description.rst Outdated Show resolved Hide resolved
docs/jwst/undersampling_correction/description.rst Outdated Show resolved Hide resolved
docs/jwst/undersampling_correction/description.rst Outdated Show resolved Hide resolved
@jotaylor jotaylor force-pushed the undersampling_docs branch from 625bf88 to d63c0cd Compare May 19, 2023 18:18
@jotaylor
Copy link
Contributor Author

Thanks for the comments @hbushouse ! I think I've addressed them all, but please let me know if anything else needs updated.

Copy link
Collaborator

@hbushouse hbushouse left a comment

Choose a reason for hiding this comment

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

Couple of errors in syntax for the ramp_fit links need fixing.

docs/jwst/undersampling_correction/description.rst Outdated Show resolved Hide resolved
docs/jwst/undersampling_correction/description.rst Outdated Show resolved Hide resolved
@jotaylor jotaylor force-pushed the undersampling_docs branch from a60423e to 09d3433 Compare May 25, 2023 17:25
Copy link
Collaborator

@hbushouse hbushouse left a comment

Choose a reason for hiding this comment

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

Looks good. All of the links now work correctly in the new docs.

@hbushouse hbushouse merged commit 2687be7 into spacetelescope:master May 26, 2023
mairanteodoro pushed a commit to mairanteodoro/jwst that referenced this pull request Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants