Skip to content

fix case of single template#346

Merged
sbailey merged 1 commit into
mainfrom
template1
Jan 27, 2026
Merged

fix case of single template#346
sbailey merged 1 commit into
mainfrom
template1

Conversation

@sbailey
Copy link
Copy Markdown
Collaborator

@sbailey sbailey commented Jan 24, 2026

This PR fixes #345. Instead of recasting the 1D coeff column back to 2D, it fixes the upstream code that was incorrectly causing the 1D coeff column in the first place. At the same time, if allzfit['coeff'].ndim == 1 (maybe possible with archetypes?), then it was a bug anyway to have ntarg = allzfit['coeff'].shape instead of ntarg = allzfit['coeff'].shape[0], so I fixed that here too.

Tested that this produces the same answer as main for default templates

source /global/common/software/desi/desi_environment.sh main
cd /pscratch/sd/s/sjbailey/desi/redrock/template1
salloc -N 1 -C cpu -t 04:00:00 -q interactive

# main branch, standard templates
srun -n 64 rrdesi_mpi \
    --zscan-galaxy=1.2,1.3,1e-4 \
    -i /global/cfs/cdirs/desi/spectro/redux/daily/tiles/cumulative/83582/20260116/coadd-4-83582-thru20260116.fits \
    -o redrock-main.fits \
    --templates /global/common/software/desi/perlmutter/desiconda/20240425-2.2.0/code/redrock-templates/main/

# this branch, standard templates
module swap redrock/sjb
srun -n 64 rrdesi_mpi \
    --zscan-galaxy=1.2,1.3,1e-4 \
    -i /global/cfs/cdirs/desi/spectro/redux/daily/tiles/cumulative/83582/20260116/coadd-4-83582-thru20260116.fits \
    -o redrock-branch.fits \
    --templates /global/common/software/desi/perlmutter/desiconda/20240425-2.2.0/code/redrock-templates/main/

# compare
fitsdiff redrock-main.fits redrock-branch.fits

--> bitwise identical for data, only differing in header timestamps and vesion tracking keywords

fixes problem reported in #345

srun -n 64 rrdesi_mpi \
    --zscan-galaxy=1.2,1.3,1e-4 \
    -i /global/cfs/cdirs/desi/spectro/redux/daily/tiles/cumulative/83582/20260116/coadd-4-83582-thru20260116.fits \
    -o redrock-lae.fits \
    --templates /global/cfs/cdirs/desicollab/users/rongpu/data/redrock/lae_templates_20260120

(note that compared to original report I restricted redshift range for faster testing of array dimensionality).

@coveralls
Copy link
Copy Markdown

Coverage Status

coverage: 42.482%. remained the same
when pulling 4e42918 on template1
into 5705e67 on main.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes issue #345 where redrock encounters errors when running with only a single template spectrum. The root cause was that the coeff column was being incorrectly flattened from 2D to 1D when it had only one coefficient, breaking downstream code that expects 2D arrays.

Changes:

  • Prevents automatic flattening of the coeff array when it has only one coefficient (shape[1] == 1)
  • Fixes a bug where a tuple was incorrectly assigned to ntarg instead of extracting the integer from shape[0]

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown
Contributor

@rongpu rongpu 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 for identifying the source of this bug!

@sbailey sbailey merged commit 5d4cdf3 into main Jan 27, 2026
22 checks passed
@sbailey sbailey deleted the template1 branch January 27, 2026 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug in zfind.py when there is only one template spectrum

4 participants