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

Merged
merged 12 commits into from
Oct 15, 2024

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 (1ae172e) to head (232c2e1).
Report is 1 commits behind head on main.

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 1ae172e

*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.

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 Dave. That looks good. I left one inline comment re whether or not to convert projcell_info to a string.

As follow-up work, could we add the following features?

  • Let's extend the projcell info to include the orientat_cent from orientat_projection_center. This is presently always zero, and for now we can just throw an error if it's not equal to that, but it's the one bit of information we're not tracking.
  • Let's write the code to actually use this wcs info in resample.
  • Presently this code generates the "visit" products, though it kind of just assumes that you pass it a visit. It would be good if you just passed it a list of files and it figured out how to make the various pass / visit / full combinations. I think you could generate those with an additional loop over these, something like the following pseudocode:
obsid_list = []
for file_name in filelist:
    obsid_list.append(cal_file.meta.observation.obs_id)
visitid = [o[:19] for o in obsid_list]
passid = [o[:10] for o in obsid_list]
fullid = [o[:5] for o in obsid_list]
# generate visit products
for visit in unique(visitid):
    generate_products([f for f, v in zip(filelist, visitid) if v == visit], type='visit')
# generate pass products
for passno in unique(passid):
    generate_products([f for f, p in zip(filelist, passid) if p == passno], type='pass')
for progno in np.unique(fullid):
    generate_products([f for f, p in zip(filelist, fullid) if p == progno), type='full')

The other thing that we would need to loop over would be the filter names, so that generate_products only sees one filter at a time.

prompt_product_asn["asn_type"] = "image"
prompt_product_asn["program"] = program_id
prompt_product_asn["target"] = patch_name
prompt_product_asn["skycell_wcs_info"] = json.dumps(projcell_info)
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 I would have imagined leaving this as a dictionary, rather than converting it to a string via dumps? What's the motivation for stringifying this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Keeping this a string allows the other unit tests to pass since we're using one json schema file at the moment. Since a dictionary is more convenient we should add schema for the skycell asn - at a later date.

@ddavis-stsci ddavis-stsci merged commit 09e3844 into spacetelescope:main Oct 15, 2024
27 checks passed
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
3 participants