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.

Sign up for GitHub

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

Jump to bottom

RCAL-941: Update romancal to use new L1/L2 schema. #1473

Merged
merged 26 commits into from
Oct 29, 2024
Merged

RCAL-941: Update romancal to use new L1/L2 schema. #1473

merged 26 commits into from
Oct 29, 2024

Conversation

mairanteodoro
Copy link
Collaborator

@mairanteodoro mairanteodoro commented Oct 23, 2024 •
edited
Loading

Resolves RCAL-941

This PR updates romancal to comply with the changes in L1/L2 schema. All the unit and regression tests have been updated.

Regression Tests

Tasks

  • request a review from someone specific, to avoid making the maintainers review every PR
  • add a build milestone, i.e. 24Q4_B15 (use the latest build if not sure)
  • Does this PR change user-facing code / API? (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)
    • update or add relevant tests
    • update relevant docstrings and / or docs/ page
    • start a regression test and include a link to the running job (click here for instructions)
      • Do truth files need to be updated ("okified")?
        • after the reviewer has approved these changes, run okify_regtests to update the truth files
  • if a JIRA ticket exists, make sure it is resolved properly
news fragment change types...
  • changes/<PR#>.general.rst: infrastructure or miscellaneous change
  • changes/<PR#>.docs.rst
  • changes/<PR#>.stpipe.rst
  • changes/<PR#>.associations.rst
  • changes/<PR#>.scripts.rst
  • changes/<PR#>.mosaic_pipeline.rst
  • changes/<PR#>.patch_match.rst

steps

  • changes/<PR#>.dq_init.rst
  • changes/<PR#>.saturation.rst
  • changes/<PR#>.refpix.rst
  • changes/<PR#>.linearity.rst
  • changes/<PR#>.dark_current.rst
  • changes/<PR#>.jump_detection.rst
  • changes/<PR#>.ramp_fitting.rst
  • changes/<PR#>.assign_wcs.rst
  • changes/<PR#>.flatfield.rst
  • changes/<PR#>.photom.rst
  • changes/<PR#>.flux.rst
  • changes/<PR#>.source_detection.rst
  • changes/<PR#>.tweakreg.rst
  • changes/<PR#>.skymatch.rst
  • changes/<PR#>.outlier_detection.rst
  • changes/<PR#>.resample.rst
  • changes/<PR#>.source_catalog.rst

@mairanteodoro mairanteodoro force-pushed the RCAL-941-update-romancal-to-use-new-l1-l2-schema branch from 2bc8ed0 to b4c9108 Compare October 23, 2024 20:25
@codecov Codecov
Copy link

codecov bot commented Oct 23, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 96.42857% with 1 line in your changes missing coverage. Please review.

Project coverage is 76.19%. Comparing base (07d66cf) to head (c732d3b).
Report is 27 commits behind head on main.

Files with missing lines Patch % Lines
romancal/jump/jump_step.py 75.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1473      +/-   ##
==========================================
+ Coverage   76.15%   76.19%   +0.04%     
==========================================
  Files         115      115              
  Lines        7638     7626      -12     
==========================================
- Hits         5817     5811       -6     
+ Misses       1821     1815       -6

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

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Oct 24, 2024
schlafly
schlafly reviewed Oct 24, 2024
View reviewed changes
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.

A few comments inline, thanks!

romancal/ramp_fitting/ramp_fit_step.py Outdated Show resolved Hide resolved
romancal/resample/resample.py Outdated Show resolved Hide resolved
romancal/resample/resample.py Show resolved Hide resolved
romancal/resample/resample_step.py Outdated Show resolved Hide resolved
@mairanteodoro mairanteodoro added this to the 25Q1_B16 milestone Oct 28, 2024
schlafly
schlafly reviewed Oct 28, 2024
View reviewed changes
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.

This looks good to me. A few very minor comments inline.

romancal/flatfield/flat_field.py Outdated
else:
apply_flat_field(output_model, flat_model)
output_model.meta.cal_step.flat_field = "COMPLETE"
output_model.meta.cal_step["flat_field"] = "COMPLETE"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would you mind explaining this change? I think I thought that flat_field was in the schema and could use the "." notation?

romancal/jump/jump_step.py
frames_per_group = meta.exposure.nframes
# FIXME: since frames_per_group => meta.exposure.nframes has been removed,
# we need to fix stcal.jump.jump to remove it from there too
frames_per_group = 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that we don't actually use this jump mode any longer; we should probably just open a ticket to remove this step. I'll do that now.

But for this ticket this is fine.

romancal/stpipe/core.py
# convert to model.cal_logs type to avoid validation errors
model.meta.cal_logs = type(model.meta.cal_logs)(self.log_records)
if isinstance(model, MosaicModel):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should put them under meta in both cases, but this is good for now; we can delay until the coming L3 metadata refactoring.

PaulHuwe
PaulHuwe approved these changes Oct 29, 2024
View reviewed changes
Copy link
Collaborator

@PaulHuwe PaulHuwe left a comment

Choose a reason for hiding this comment

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

LGTM

@mairanteodoro mairanteodoro marked this pull request as ready for review October 29, 2024 18:51
@mairanteodoro mairanteodoro requested a review from a team as a code owner October 29, 2024 18:51
schlafly
schlafly approved these changes Oct 29, 2024
View reviewed changes
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.

Thanks, looks good!

@mairanteodoro mairanteodoro merged commit 4c078bf into spacetelescope:main Oct 29, 2024
31 checks passed
@stscijgbot-rstdms stscijgbot-rstdms mentioned this pull request Oct 31, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PaulHuwe PaulHuwe PaulHuwe approved these changes

@schlafly schlafly schlafly approved these changes

Assignees
No one assigned
Labels
Dark Current dependencies Pull requests that update a dependency file documentation Improvements or additions to documentation dq_init flatfield jump linearity Photom pipeline ramp_fitting regression_testing Saturation stpipe testing Wide Field Instrument (WFI)
Projects
None yet
Milestone
25Q1_B16
Development

Successfully merging this pull request may close these issues.
3 participants
@mairanteodoro @schlafly @PaulHuwe