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

rcal-920 script to create associations based on skycells #1437

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

ddavis-stsci
Copy link
Collaborator

@ddavis-stsci ddavis-stsci commented Oct 3, 2024

Resolves RCAL-920

Closes #1421

This PR adds code to take an input list of calibrated WFI exposures and creates associations based on the
skycells that they overlap.

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

Regression test results are at
https://github.com/spacetelescope/RegressionTests/actions/runs/11168259268

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

Comment on lines 10 to 16
def passfail(bool_expr):
"""set pass fail"""
if bool_expr:
return "Pass"
return "Fail"


Copy link
Collaborator

@braingram braingram Oct 3, 2024

Choose a reason for hiding this comment

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

Suggested change
def passfail(bool_expr):
"""set pass fail"""
if bool_expr:
return "Pass"
return "Fail"

No longer needed with the suggestion below: #1437 (comment)

Comment on lines 44 to 47
for file in output_files:
skycell_asn.logger.info(
"Check that the json file exists " + passfail(os.path.isfile(file))
)
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
for file in output_files:
skycell_asn.logger.info(
"Check that the json file exists " + passfail(os.path.isfile(file))
)
missing_files = [fn for fn in output_files if not os.path.isfile(fn)]
assert not missing_files, f"Missing files: {missing_files}"

How does this look for an assert to add to this test? Without an assert that checks there are no missing files the test can silently pass if one or all of the files are missing.

Copy link

codecov bot commented Oct 3, 2024

Codecov Report

Attention: Patch coverage is 43.75000% with 36 lines in your changes missing coverage. Please review.

Project coverage is 78.09%. Comparing base (2383105) to head (6a33a80).

Files with missing lines Patch % Lines
romancal/associations/skycell_asn.py 41.93% 36 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1437      +/-   ##
==========================================
- Coverage   78.38%   78.09%   -0.29%     
==========================================
  Files         118      119       +1     
  Lines        7680     7744      +64     
==========================================
+ Hits         6020     6048      +28     
- Misses       1660     1696      +36     
Flag Coverage Δ *Carryforward flag
nightly 62.12% <ø> (ø) Carriedforward from 2383105

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

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

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.

Utility to create asn files based on skycells
2 participants