jwst
jwst copied to clipboard
Include IRAFStarFinder option for tweakreg_catalog
This PR allows users to swap between the default DAOStarFinder and IRAFStarFinder for source catalog in the tweakreg step. In some cases (particularly for shorter bands where the PSF is more undersampled), I've found this produces better results, and this is also seen in some of the discussion in #6295. It now gets exposed as a switch 'dao'/'iraf', but the other parameters are passed to it exactly the same way as DAOStarFinder
Checklist for maintainers
- [x] added entry in
CHANGES.rstwithin the relevant release section - [ ] updated or added relevant tests
- [x] updated relevant documentation
- [x] added relevant milestone
- [x] 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
Codecov Report
Patch coverage: 25.00% and project coverage change: -0.02 :warning:
Comparison is base (
930628f) 77.60% compared to head (81c18f5) 77.59%.
Additional details and impacted files
@@ Coverage Diff @@
## master #7024 +/- ##
==========================================
- Coverage 77.60% 77.59% -0.02%
==========================================
Files 452 452
Lines 36193 36197 +4
==========================================
- Hits 28088 28086 -2
- Misses 8105 8111 +6
| Flag | Coverage Δ | *Carryforward flag | |
|---|---|---|---|
| nightly | 77.61% <ø> (-0.01%) |
:arrow_down: | Carriedforward from 930628f |
| unit | 49.72% <25.00%> (-0.01%) |
:arrow_down: |
*This pull request uses carry forward flags. Click here to find out more.
| Impacted Files | Coverage Δ | |
|---|---|---|
| jwst/tweakreg/tweakreg_step.py | 61.31% <ø> (ø) |
|
| jwst/tweakreg/tweakreg_catalog.py | 69.56% <25.00%> (-25.18%) |
:arrow_down: |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
I think it would be useful to get @larrybradley opinion.
My main concern is a constantly growing number of parameters in this automated pipeline step. IMO, users would benefit greatly from using stand-alone data analysis packages photutils (or SExtractor or whatever they choose) and tweakwcs.
For example, previously it was requested to support user-provided catalogs. There is a PR for that: https://github.com/spacetelescope/jwst/pull/7022 In this context, my philosophical question is whether it makes sense to increase the number of options for this step or should the users use photutils to create custom catalogs and pass them to the tweakreg step...
This PR looks fine from the technical point of view so I am going to approve it but I will leave the decision to merge it to @hbushouse
There are many ways users may want to make specialized catalogs (star finders, segmentation based, PSF fitting, etc.)
Just to add to the list, see https://github.com/spacetelescope/jwst/issues/6962
I agree with these comments, but while a catalogue can be passed to tweakreg for the final step aligning everything to an 'absolute' frame, there isn't such functionality to pass either a catalogue function or individual catalogues for each tile for the relative alignment at the moment
... while a catalogue can be passed to tweakreg for the final step aligning everything to an 'absolute' frame, there isn't such functionality to pass either a catalogue function or individual catalogues for each tile for the relative alignment at the moment
That is exactly what https://github.com/spacetelescope/jwst/pull/7022 will do (shortly). In my personal experience, IRAFStarFind is not as important or accurate as DAOStarFind and it might require different values for other parameters. And as @larrybradley mentioned above, there are many other ways of finding sources, including external packages such as SExtractor.
Given that https://github.com/spacetelescope/jwst/issues/7022 is about to be merged soon, I do not see the need for this additional complication.
@thomaswilliamsastro Sorry for the late response. We discussed the proposed change internally and decided not to accept it. The pipeline now supports custom user-provided catalogs not only for the absolute astrometric alignment but also for the relative alignment - see #7022 . In addition, we don't want to increase the complexity of the interface and think adding another parameter is undesirable.