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

update SourceCatalog meta data handling for new photutils changes #8014

Closed
wants to merge 3 commits into from

Conversation

braingram
Copy link
Collaborator

@braingram braingram commented Oct 19, 2023

The development version of photutils updated what is contained in the SourceCatalog meta data. One specific change that is impacting jwst is changing version to versions in the meta.
astropy/photutils#1640

This is causing devdeps failures:
https://github.com/spacetelescope/jwst/actions/runs/6562032419/job/17823126866?pr=8010#step:10:339

This PR updates set_segment_properties to reproduce version from the new versions key in the SourceCatalog meta.

Regression tests (with released version of photutils) running at: https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1048/

Checklist for maintainers

  • added entry in CHANGES.rst within the relevant release section
  • updated or added relevant tests
  • updated relevant documentation
  • added relevant milestone
  • added relevant label(s)
  • ran regression tests, post a link to the Jenkins job below.
    How to run regression tests on a PR
  • Make sure the JIRA ticket is resolved properly

if 'versions' in segm_cat.meta:
segm_cat.meta['version'] = segm_cat.meta['versions']
del segm_cat.meta['versions']

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a more elegant way to do this by somehow storing either version or versions (whichever appears in the meta data) in a local variable (or something like that) that then gets used in subsequent code? Not sure I like the idea of forcing the code to continue using the old entry forever.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The segm_cat meta gets merged with SourceCatalog.meta:

self.meta.update(segm_cat.meta)

which is later used to generate the returned catalog meta:
self.meta['version']['jwst'] = jwst_version
self.meta['aperture_params'] = self.aperture_params
self.meta['abvega_offset'] = self.abvega_offset
catalog.meta.update(self.meta)

which appears to be the primary output of SourceCatalogStep (and is optionally saved as a .ecsv file).

Without correcting the segm_cat.meta to use version instead of versions I think this would mean that the output of the SourceCatalogStep and the saved .ecsv file would differ (including versions instead of version).

Running calwebb_image3 on the level3_F277W_3img_asn.json used for the test_tweakreg_with_gaia regression test I did confirm that version is written to the LMC_F277W_modA_dither_mosaic_cat.ecsv produced during the run. I haven't been able to find any unit or regression test that specifically checks for this version metadata so I don't expect that changing it would cause any tests to fail but would change the output.

Copy link
Collaborator Author

@braingram braingram Oct 23, 2023

Choose a reason for hiding this comment

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

If the changed output is acceptable I can look at updating this pr to use either version or versions and produce a file that for all released versions of photutils would produce a file with version and for all new versions produce a file with `versions. @hbushouse is it preferable to match the new output of photutils or match the previous output produced from the jwst pipeline?

@codecov
Copy link

codecov bot commented Oct 19, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Files Coverage Δ
jwst/tweakreg/tweakreg_step.py 63.54% <100.00%> (+1.86%) ⬆️
jwst/source_catalog/source_catalog.py 97.49% <33.33%> (-0.45%) ⬇️

📢 Thoughts on this report? Let us know!

@braingram braingram mentioned this pull request Oct 23, 2023
7 tasks
Co-authored-by: Mihai Cara <[email protected]>
@braingram braingram marked this pull request as ready for review November 7, 2023 14:22
@braingram braingram requested a review from a team as a code owner November 7, 2023 14:22
@larrybradley
Copy link
Member

Why is this necessary? The meta dict in the output catalog is informational only. While technically this is a breaking change, no one should be accessing that dict in downstream code.

@braingram
Copy link
Collaborator Author

Why is this necessary? The meta dict in the output catalog is informational only. While technically this is a breaking change, no one should be accessing that dict in downstream code.

Thanks for taking a look. The meta dictionary is both copied into the source catalog meta data and modified here in the jwst source catalog step:

self.meta.update(segm_cat.meta)
for key in ('sklearn', 'matplotlib'):
self.meta['version'].pop(key)

The change to use versions (instead of version) causes the modifications to fail with a KeyError (as seen in the devdeps testing on main.

@larrybradley
Copy link
Member

larrybradley commented Nov 7, 2023

Instead, that block of code (L238-240) should be removed or updated. The intent was to not include optional dependencies not used in the pipeline. However, since I originally wrote that code block long ago, photutils has added new optional dependencies which are not currently being removed. Either we should simply remove that code block, or it should be updated to only include (instead of exclude) certain packages for which we want to log the versions.

I'm happy to do that in a new PR.

@larrybradley
Copy link
Member

I'm going to revert this change in photutils and postpone it for a future release. I was planning a release soon, and I don't want to break the pipeline. I didn't expect that change to break anything, but of course it was something that I wrote in the pipeline long ago. 😅 In the meantime, I'll update the code here to prepare for that change in a future photutils release.

@braingram
Copy link
Collaborator Author

Let me know if there's anything I can do to help. I'll convert this to draft if that works for you.

@braingram braingram marked this pull request as draft November 7, 2023 19:11
@braingram
Copy link
Collaborator Author

Closing in favor of #8066

@braingram braingram closed this Nov 8, 2023
@braingram braingram deleted the photutils_versions branch November 8, 2023 19:00
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.

4 participants