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

adjust coeff size for legendre only for archetypes #266

Merged
merged 2 commits into from Jan 5, 2024
Merged

Conversation

sbailey
Copy link
Collaborator

@sbailey sbailey commented Jan 4, 2024

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.

@sbailey sbailey requested a review from abhi0395 January 4, 2024 23:31
@coveralls
Copy link

coveralls commented Jan 4, 2024

Coverage Status

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

Copy link
Member

@abhi0395 abhi0395 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!

@sbailey
Copy link
Collaborator Author

sbailey commented Jan 5, 2024

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.

@sbailey sbailey merged commit 36ebd46 into main Jan 5, 2024
12 checks passed
@sbailey sbailey deleted the num_coeff branch January 5, 2024 00:00
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.

None yet

3 participants