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

Convert tweakreg segmentation connectivity to integer #8309

Merged
merged 3 commits into from
Feb 28, 2024

Conversation

jdavies-st
Copy link
Collaborator

@jdavies-st jdavies-st commented Feb 26, 2024

This PR fixes a bug when using the new SourceFinder (via segmentation) starfinder in tweakreg which was added in #8203. ConfigObj option always returns a string, and that causes problems when one wants options that are ints or floats. They need to be specifically cast to the correct type.

Another reason to stop using ConfigObj. ;-)

The bug is the following:

    TweakRegStep.call(files_to_tweak, save_results=True, suffix="cal",
  File "/home/jdavies/miniconda3/envs/jwst/lib/python3.11/site-packages/stpipe/step.py", line 653, in call
    return instance.run(*args)
           ^^^^^^^^^^^^^^^^^^^
  File "/home/jdavies/miniconda3/envs/jwst/lib/python3.11/site-packages/stpipe/step.py", line 479, in run
    step_result = self.process(*args)
                  ^^^^^^^^^^^^^^^^^^^
  File "/home/jdavies/miniconda3/envs/jwst/lib/python3.11/site-packages/jwst/tweakreg/tweakreg_step.py", line 208, in process
    catalog = make_tweakreg_catalog(
              ^^^^^^^^^^^^^^^^^^^^^^
  File "/home/jdavies/miniconda3/envs/jwst/lib/python3.11/site-packages/jwst/tweakreg/tweakreg_catalog.py", line 220, in make_tweakreg_catalog
    sources = StarFinder(model.data, threshold, mask=coverage_mask, **starfinder_kwargs)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/jdavies/miniconda3/envs/jwst/lib/python3.11/site-packages/jwst/tweakreg/tweakreg_catalog.py", line 56, in _SourceFinderWrapper
    segment_map = finder(data, threshold, mask=mask)
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/jdavies/miniconda3/envs/jwst/lib/python3.11/site-packages/photutils/segmentation/finder.py", line 191, in __call__
    segment_img = detect_sources(data, threshold, self.npixels, mask=mask,
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/jdavies/miniconda3/envs/jwst/lib/python3.11/site-packages/astropy/utils/decorators.py", line 604, in wrapper
    return function(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/jdavies/miniconda3/envs/jwst/lib/python3.11/site-packages/photutils/segmentation/detect.py", line 396, in detect_sources
    footprint = _make_binary_structure(data.ndim, connectivity)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/jdavies/miniconda3/envs/jwst/lib/python3.11/site-packages/photutils/segmentation/utils.py", line 96, in _make_binary_structure
    raise ValueError(f'Invalid connectivity={connectivity}.  '
ValueError: Invalid connectivity=8.  Options are 4 or 8.

One may want to update one of the regtests to use segmentation as the starfinder. 2 other things for comment:

The current starfinder kwarg in spec is fine, but sourcefinder would be better, as most sources in JWST images are not stars, but galaxies. The more general term might be more accurate.

Currently the segmentation method doesn't convolve the image first before running detection. This is done in the standard photutils docs and in SourceCatalogStep, so perhaps this was an oversight and should be added. This is needed to boost S/N of detections.

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

Copy link

codecov bot commented Feb 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.21%. Comparing base (4cc0ac1) to head (61b31fc).
Report is 1 commits behind head on master.

❗ Current head 61b31fc differs from pull request most recent head 07154b3. Consider uploading reports for the commit 07154b3 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8309      +/-   ##
==========================================
+ Coverage   75.15%   75.21%   +0.05%     
==========================================
  Files         470      474       +4     
  Lines       38604    38810     +206     
==========================================
+ Hits        29014    29192     +178     
- Misses       9590     9618      +28     
Flag Coverage Δ *Carryforward flag
nightly 77.37% <ø> (-0.03%) ⬇️ Carriedforward from 4cc0ac1

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

@larrybradley larrybradley left a comment

Choose a reason for hiding this comment

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

Thanks, @jdavies-st

Copy link
Collaborator

@emolter emolter left a comment

Choose a reason for hiding this comment

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

looks good, thanks!

@hbushouse
Copy link
Collaborator

@hbushouse hbushouse added this to the Build 10.2 milestone Feb 27, 2024
@hbushouse
Copy link
Collaborator

Regression test run is clean, as expected. Merging.

@hbushouse hbushouse merged commit 7750d76 into spacetelescope:master Feb 28, 2024
23 of 24 checks passed
@jdavies-st jdavies-st deleted the fix-tweakreg-segmentation branch February 28, 2024 14:56
emolter pushed a commit to emolter/jwst that referenced this pull request Feb 28, 2024
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.

None yet

4 participants