Repository navigation
Conversation
…variable in astrsolve Two bugs fixed: 1. mosaic.c: SAME_CRVAL overwrote all detectors' CRVALs with the field mean without adjusting CRPIX or CD. This collapsed all detectors to the same sky position, breaking the virtual-image matching step for multi-detector instruments. Now uses crval_to_crpix() like the other mosaic types, which preserves each detector's sky footprint while sharing a common tangent point. Tested with VIRCAM 16-detector data (30 exposures): internal astrometric residuals went from 1.46" (broken) to 0.018" (fixed), matching the performance of LOOSE and FIX_FOCALPLANE modes. 2. astrsolve.c: In the loop computing findex2 adjustments per instrument, the code used a stale `instru` variable (set by the previous loop's last field) instead of the loop variable `i`. This is harmless for single-instrument setups but would corrupt the normal equation matrix dimensions for multi-instrument configurations.
ebertin
requested changes
Apr 5, 2026
ebertin
left a comment
ebertin
left a comment
Member
There was a problem hiding this comment.
Thank you for your PR! You will find my comments inline.
Comment on lines
-113
to
+114
| if (prefs.mosaic_type[field->astromlabel] == MOSAIC_SAMECRVAL) | ||
| for (s=0; s<field->nset; s++) | ||
| { | ||
| for (d=0; d<naxis; d++) | ||
| sets[s]->wcs->crval[d] = wcsmean[d]; | ||
| init_wcs(sets[s]->wcs); | ||
| range_wcs(sets[s]->wcs); | ||
| invert_wcs(sets[s]->wcs); | ||
| } | ||
| else | ||
| for (s=0; s<field->nset; s++) | ||
| crval_to_crpix(sets[s]->wcs, wcsmean); | ||
| for (s=0; s<field->nset; s++) | ||
| crval_to_crpix(sets[s]->wcs, wcsmean); |
Member
There was a problem hiding this comment.
I think you misunderstood the goal of the MOSAIC_TYPE SAME_CRVAL option, which is to bring the CRVALs of all extensions to a common set of coordinates without compensating the possible shifts using the CDs and CRPIXs. It is generally used in conjunction with a .ahead file defining the focal plane of a mosaic instrument (where the CRPIXs and CDs are known to be correct), when the original CRVALs have been re-adjusted individually (common among simple astrometry calibration pipelines). What you actually want is what the MOSAIC_TYPE SHARE_PROJAXIS option already provides (the "else" code path here). So, in short, no code change is required here.
| for (i=0; i<ninstru; i++) | ||
| { | ||
| if (prefs.stability_type[instru]!=STABILITY_PREDISTORTED) | ||
| if (prefs.stability_type[i]!=STABILITY_PREDISTORTED) |
smeingast
added a commit
to smeingast/scamp
that referenced
this pull request
Apr 9, 2026
In astrsolve_fgroups() the loop at src/astrsolve.c:273 iterates `i` from 0 to ninstru, but the stability_type[] check inside used the variable `instru` (set at line 193 to field->astromlabel from the previous loop) instead of the current loop variable `i`. For single-instrument setups the two indices match and this has no effect. For multi-instrument configurations with mixed STABILITY_TYPE values per instrument, the check would consult the wrong instrument and potentially mis-size the normal equation matrix. Fix: use `i` as the index into prefs.stability_type. Split out from astromatic#38; the mosaic.c portion of that PR is being dropped (SHARE_PROJAXIS already provided the intended behaviour).
Contributor
Author
|
Thank you for the detailed review @ebertin. You're right on both counts:
Closing this PR in favour of #39. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
MOSAIC_TYPE SAME_CRVALoverwrote all detectors' CRVALs with the field mean without adjusting CRPIX or CD. This collapsed all detectors to the same sky position, breaking the virtual-image matching for multi-detector instruments (e.g. VIRCAM). Now usescrval_to_crpix()like the other mosaic types, preserving each detector's sky footprint while sharing a common tangent point.findex2adjustments per instrument (line 275), the code used a staleinstruvariable from the previous loop instead of the current loop variablei. Harmless for single-instrument setups but would corrupt the normal equation matrix for multi-instrument configurations.Test results (VIRCAM, 16 detectors, 30 exposures)
All modes now produce equivalent results with
STABILITY_TYPE INSTRUMENT.Test plan
astrsolveindexing is unchangedastrsolvefix not tested (no multi-instrument data available)