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

TweakReg: doc updates. #673

Conversation

mairanteodoro
Copy link
Collaborator

@mairanteodoro mairanteodoro commented Apr 6, 2023

Resolves RCAL-527

This PR addresses updates to the RomanCal version of TweakReg.

Checklist

  • added entry in CHANGES.rst under the corresponding subsection
  • updated relevant tests
  • updated relevant documentation
  • updated relevant milestone(s)
  • added relevant label(s)

@mairanteodoro mairanteodoro requested a review from a team as a code owner April 6, 2023 19:02
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Apr 6, 2023
@codecov
Copy link

codecov bot commented Apr 6, 2023

Codecov Report

Patch coverage: 92.85% and project coverage change: +0.81 🎉

Comparison is base (b7ceecd) 63.74% compared to head (6d92ea5) 64.56%.

❗ Current head 6d92ea5 differs from pull request most recent head c0c25dd. Consider uploading reports for the commit c0c25dd to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #673      +/-   ##
==========================================
+ Coverage   63.74%   64.56%   +0.81%     
==========================================
  Files          78       78              
  Lines        4115     4117       +2     
==========================================
+ Hits         2623     2658      +35     
+ Misses       1492     1459      -33     
Flag Coverage Δ *Carryforward flag
nightly 63.64% <ø> (+0.12%) ⬆️ Carriedforward from 587c482

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

Impacted Files Coverage Δ
romancal/tweakreg/tweakreg_step.py 62.92% <92.30%> (+12.54%) ⬆️
romancal/tweakreg/astrometric_utils.py 92.42% <100.00%> (+0.11%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

@mairanteodoro mairanteodoro requested a review from schlafly April 10, 2023 13:09
Copy link
Collaborator

@schlafly schlafly 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. Two minor questions about tweakreg features that make sense at L3 time but which I don't really understand at L2 time. One other thing that would be nice would be an example invocation of the pipeline with a custom catalog. There's a lot of code documentation there but not very much user-facing documentation there.

@github-actions github-actions bot added pipeline testing dependencies Pull requests that update a dependency file labels Apr 12, 2023
@mairanteodoro mairanteodoro force-pushed the RCAL-527-update-tweakreg-documentation branch from 55a717c to 04c3f3f Compare April 12, 2023 20:01
@github-actions github-actions bot removed the dependencies Pull requests that update a dependency file label Apr 12, 2023
@mairanteodoro mairanteodoro requested a review from schlafly April 12, 2023 20:07
Copy link
Collaborator

@schlafly schlafly left a comment

Choose a reason for hiding this comment

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

These docs still look good to me. I am a bit confused, though---mostly in my last review I wanted some clarification about what a couple things did when tweakreg is used at L2 time, which is the default case for Roman. There we don't have overlaps and some steps presumably get skipped. I think we should make this default case very clear.

I also thought that some user facing documentation of the custom catalog option would be valuable, since a lot of the docs talked about the API but as a user I would have liked to see e.g. an invocation that used that feature. I don't think that additional documentation is strictly necessary if it's hard or if we don't have a CLI to that bit of the step, though.

@mairanteodoro mairanteodoro force-pushed the RCAL-527-update-tweakreg-documentation branch from 04c3f3f to 18546a1 Compare April 14, 2023 20:56
@github-actions github-actions bot removed testing documentation Improvements or additions to documentation pipeline labels Apr 14, 2023
@mairanteodoro mairanteodoro reopened this Apr 17, 2023
@github-actions github-actions bot added dependencies Pull requests that update a dependency file documentation Improvements or additions to documentation pipeline testing labels Apr 17, 2023
@github-actions github-actions bot removed the dependencies Pull requests that update a dependency file label Apr 17, 2023
@mairanteodoro
Copy link
Collaborator Author

These docs still look good to me. I am a bit confused, though---mostly in my last review I wanted some clarification about what a couple things did when tweakreg is used at L2 time, which is the default case for Roman. There we don't have overlaps and some steps presumably get skipped. I think we should make this default case very clear.

I also thought that some user facing documentation of the custom catalog option would be valuable, since a lot of the docs talked about the API but as a user I would have liked to see e.g. an invocation that used that feature. I don't think that additional documentation is strictly necessary if it's hard or if we don't have a CLI to that bit of the step, though.

We added an "Examples" page to the TweakReg docs with three scenarios, including the case where the user provides the source catalog instead of using the one provided by SourceDetection.

@mairanteodoro mairanteodoro requested a review from nden April 17, 2023 15:52
@mairanteodoro mairanteodoro merged commit a8b3a72 into spacetelescope:main Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants