-
Notifications
You must be signed in to change notification settings - Fork 169
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
Undersampling docs #7589
Conversation
Codecov ReportPatch coverage:
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
*This pull request uses carry forward flags. Click here to find out more.
☔ View full report in Codecov by Sentry. |
@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
(yes, all the dots, underscores, and colon are required). Then in the undersampling page insert a reference/link to that target using
(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. |
There was a problem hiding this 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).
625bf88
to
d63c0cd
Compare
Thanks for the comments @hbushouse ! I think I've addressed them all, but please let me know if anything else needs updated. |
There was a problem hiding this 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.
a60423e
to
09d3433
Compare
There was a problem hiding this 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.
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-migrationCan someone please tell me how to do so?
Checklist for maintainers
CHANGES.rst
within the relevant release sectionHow to run regression tests on a PR