search

desihub / redrock

Redshift fitting for spectroperfectionism
BSD 3-Clause "New" or "Revised" License
21 stars 13 forks source link

adjust coeff size for legendre only for archetypes #266

Closed sbailey closed 6 months ago

sbailey commented 6 months ago

This PR is a followup to #255 (archetypes), fixing a bug where deg_legendre was being used to adjust the size of the COEFF array even if archetypes were not being used and thus no legendre polynomials either.

I didn't catch this while testing PR #255 because I had been using galaxy templates with ncoeff=10, which was greater than ncameradeg_legendre+1 = 32+1 = 7, and so the coeffs arrays remained size 10. However, when running the full pipeline, the QSOQN afterburner re-runs Redrock with only the quasar templates with ncoeff=4. Redrock was incorrectly padding this upward to 7 coefficients, which confused the afterburner expecting only 4.

@abhi0395 can you double check this? We need to get a fix in place before observations tonight. Thanks.

coveralls commented 6 months ago

Coverage Status

coverage: 33.133% (-0.06%) from 33.193% when pulling aef2c7391851f0ba03d5f444816fb7225dada03c on num_coeff into 14fd61569de156a05011f0106dab70bfcd859989 on main.

sbailey commented 6 months ago

I reviewed this in person with Abhijeet, and he made the good suggestion of also turning off legendre terms in the rrdesi wrapper when archetypes are not used. I added that.

Tested with:

cd $CFS/desi/spectro/redux/iron/tiles/cumulative/1000/20210517

# no archetypes, all templates, produces same answer as main from before #255 (redrock-main.fits)
time srun -n 4 -c 32 rrdesi_mpi --gpu -i coadd-0-1000-thru20210517.fits \
        -o $SCRATCH/redrock-all.fits

# running with only qso-HIZ template; output has 4 coeff (like before), not 7
time srun -n 4 -c 32 rrdesi_mpi --gpu -i coadd-0-1000-thru20210517.fits \
        -t $RR_TEMPLATE_DIR/rrtemplate-qso-HIZ.fits \
        -o $SCRATCH/redrock-qso.fits

For the record: in the long term I'd like to separate out the Legendre logic from the archetype logic so that we could use Legendre polynomial terms for the initial PCA/NMF scan as well. I'd also like to separate these into separate columns for the PCA/NMF coefficients, the archetype coefficients, and the legendre coefficients instead of using a single COEFF column with its meaning depending upon the context of what options were used.