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

JP-3901, JWSTKD-585: Keywords to support DHS development #426

Merged
merged 5 commits into from
Mar 6, 2025

Conversation

tapastro
Copy link
Collaborator

@tapastro tapastro commented Mar 5, 2025

Addresses JP-3901
Matches work in JWSTKD-585

This PR adds required keywords to support DHS development and processing.

Tasks

  • update or add relevant tests
  • update relevant docstrings and / or docs/ page
  • Does this PR change any API used downstream? (if not, label with no-changelog-entry-needed)
    • write news fragment(s) in changes/: echo "changed something" > changes/<PR#>.<changetype>.rst (see below for change types)
    • run jwst regression tests with this branch installed ("git+https://github.com/<fork>/stdatamodels@<branch>")
news fragment change types...
  • changes/<PR#>.feature.rst: new feature
  • changes/<PR#>.bugfix.rst: fixes an issue
  • changes/<PR#>.doc.rst: documentation change
  • changes/<PR#>.removal.rst: deprecation or removal of public API
  • changes/<PR#>.misc.rst: infrastructure or miscellaneous change

@tapastro tapastro requested a review from a team as a code owner March 5, 2025 17:07
@tapastro tapastro marked this pull request as draft March 5, 2025 17:07
Copy link

codecov bot commented Mar 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.35%. Comparing base (8b1e67c) to head (088e8a6).
Report is 7 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #426   +/-   ##
=======================================
  Coverage   78.35%   78.35%           
=======================================
  Files         109      109           
  Lines        5129     5129           
=======================================
  Hits         4019     4019           
  Misses       1110     1110           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tapastro tapastro force-pushed the jp-3901-dhs-support branch from 8b292f7 to d4efab7 Compare March 6, 2025 15:09
@tapastro tapastro marked this pull request as ready for review March 6, 2025 15:12
Copy link
Collaborator

@braingram braingram left a comment

Choose a reason for hiding this comment

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

Mostly number->integer suggestions and questions.
The titles don't seem very readable but I see those are the ones proposed in https://jira.stsci.edu/browse/JWSTKD-585
The suggestions removed abbreviations which I think improves their readability but I'm not going to press the point if it's easier to just go with the originally proposed wording.

Comment on lines 1079 to 1080
title: Num. substripe rows read starting at SUBSTRT1
type: number
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
title: Num. substripe rows read starting at SUBSTRT1
type: number
title: Number of substripe rows read starting at SUBSTRT1
type: number

Will this ever be a non-integer value? If not, any reason to not make it an integer?

Comment on lines 1083 to 1085
multistripe_skips1:
title: Num. rows to skip after reading MSTR_RD1 rows
type: number
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
multistripe_skips1:
title: Num. rows to skip after reading MSTR_RD1 rows
type: number
multistripe_skips1:
title: Number of rows to skip after reading MSTR_RD1 rows
type: number

Same as above, should this be an integer?

Comment on lines 1089 to 1090
title: Num. rows read in subsequent substripes
type: number
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
title: Num. rows read in subsequent substripes
type: number
title: Number of rows read in subsequent substripes
type: number

Same as above, integer?

Comment on lines 1094 to 1095
title: Num. rows to skip between susequent substripes
type: number
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
title: Num. rows to skip between susequent substripes
type: number
title: Number of rows to skip between subsequent substripes
type: number

Fixed spelling and should this be an integer?

Comment on lines 1099 to 1100
title: Repeat reading MSTR_RD2 rows at first position
type: number
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
title: Repeat reading MSTR_RD2 rows at first position
type: number
title: Repeat reading MSTR_RD2 rows at first position
type: integer

I think this can only be 0 or 1 (based on the doc linked in https://jira.stsci.edu/browse/JWSTKD-585).

blend_table: True
interleave_reads1:
title: Interleave MSTR_RD1 rows between substripes
type: number
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
type: number
type: integer

Same as repeat_stripe.

blend_table: True
superstripe_step:
title: Rows to advance superstripe postn btwn intgrtns
type: number
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
type: number
type: integer

fits_keyword: MSTR_ILV
blend_table: True
superstripe_step:
title: Rows to advance superstripe postn btwn intgrtns
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
title: Rows to advance superstripe postn btwn intgrtns
title: Rows to advance superstripe position between integrations.

Comment on lines 1114 to 1115
title: Num. superstripe positions in super-integration
type: number
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
title: Num. superstripe positions in super-integration
type: number
title: Number of superstripe positions in super-integration
type: integer

@tapastro
Copy link
Collaborator Author

tapastro commented Mar 6, 2025

I think the primary issue with lengthening the titles is that they will be truncated in the FITS representation. For some reason I thought number was preferred over integer, but I'll get those changed.

Copy link
Collaborator

@braingram braingram left a comment

Choose a reason for hiding this comment

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

LGTM.

If needed a follow up PR can update the titles.

@tapastro tapastro merged commit d93fdcd into spacetelescope:main Mar 6, 2025
19 checks passed
@tapastro tapastro deleted the jp-3901-dhs-support branch March 6, 2025 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants