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

QuasarNet afterburner updates part I #2402

Merged
merged 4 commits into from
Nov 5, 2024
Merged

QuasarNet afterburner updates part I #2402

merged 4 commits into from
Nov 5, 2024

Conversation

sbailey
Copy link
Contributor

@sbailey sbailey commented Oct 30, 2024

This PR requires desihub/redrock#320 and fixes:

I have not addressed #2375 in this PR because that will change the data model of the QN output in more substantive ways so I want it isolated in its own PR.

Heads up @abrodze @dylanagreen @akremin

@sbailey sbailey requested a review from abrodze October 30, 2024 23:24
@sbailey
Copy link
Contributor Author

sbailey commented Oct 30, 2024

Test output files are in /pscratch/sd/s/sjbailey/temp/qso_qn/pr2402, run with

cd $SCRATCH/temp/qso_qn
INDIR=$CFS/desi/spectro/redux/kibo/tiles/cumulative/100/20210505
for PETAL in $(seq 0 9); do
  desi_qso_qn_afterburner \
    --coadd $INDIR/coadd-$PETAL-100-thru20210505.fits \
    --redrock $INDIR/redrock-$PETAL-100-thru20210505.fits \
    --target_selection all --save_target all --delete_RR_output False \
    --output pr2402/qso_qn-$PETAL-thru20210505.fits
done

@coveralls
Copy link

coveralls commented Oct 30, 2024

Coverage Status

coverage: 30.18% (-0.01%) from 30.191%
when pulling 362dbd0 on qsoqn
into fe88bbd on main.

Copy link
Member

@akremin akremin left a comment

Choose a reason for hiding this comment

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

I'll leave the more detailed review to Allyson, since she has more in-depth knowledge of this code and how it interplays with redrock and the templates. I have two comments, one can be ignored if too much work, and one log message that I think needs to be updated.

py/desispec/scripts/qsoqn.py Outdated Show resolved Hide resolved
py/desispec/scripts/qsoqn.py Outdated Show resolved Hide resolved
@abrodze
Copy link
Member

abrodze commented Nov 1, 2024

I talked with Stephen in person about this, but I am writing it here for better tracking.
Right now, it looks like a decent percentage of the spectra being rerun through redrock are from sky fibers. I am unsure if there is a legitimate reason to keep the implementation as such, but this could easily be fixed by requiring fibermap['OBJTYPE'] == 'TGT' in the selection mask sel_index_with_QN . Though the only benefit to making such a change would be a (maybe negligible) boost in runtime, as these will still be filtered out in catalog construction.

@sbailey
Copy link
Contributor Author

sbailey commented Nov 1, 2024

I agree with @abrodze's suggestion to not rerun Redrock on sky fibers, but would like to save that for a followup PR that makes other output changes e.g. adding missing columns. I believe the only output changes from this PR are:

  • Redrock rerun files have "qn" in the name instead of "tmp" (cosmetic, but I found this cleaner in case we chose to keep them).
  • Since Redrock is run with both LOZ and HIZ templates together, the redshift overlap region is handled self consistently (instead of the HIZ answer always being picked and HIZ intermediate files overwriting the LOZ ones).

Copy link
Member

@abrodze abrodze left a comment

Choose a reason for hiding this comment

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

A few comments related to the bestchi2 section we have discussed in person. Fine to push this to future PR

py/desispec/scripts/qsoqn.py Outdated Show resolved Hide resolved
py/desispec/scripts/qsoqn.py Outdated Show resolved Hide resolved
@sbailey
Copy link
Contributor Author

sbailey commented Nov 5, 2024

@abrodze raised the additional issue of whether the simultaneous run of LOZ and HIZ templates will result in more ZWARN=4 (SMALL_DELTA_CHI2) flags in the 1.4 < z < 1.6 overlap region (close redshifts, but still |dv|>1000). Previously these would not occur because LOZ and HIZ were run separately so they couldn't trigger warnings on each other. I will try to quantify this before merging.

@sbailey
Copy link
Contributor Author

sbailey commented Nov 5, 2024

Rerun redshifts have a high ZWARN>0 rate, but they don't appear to be any higher in the 1.4 < z < 1.6 LOZ:HIZ overlap region:
image

This was based on spot checking the 10 main-dark healpix with the highest number of rerun QSOs in the overlap region.

I'm going to proceed with final cross checks on the outputs and then merge and deploy.

@sbailey sbailey merged commit 352db55 into main Nov 5, 2024
26 checks passed
@sbailey sbailey deleted the qsoqn branch November 5, 2024 23:32
@abrodze
Copy link
Member

abrodze commented Nov 6, 2024

Unless you explicitly removed them, I would guess the ~50% flags is from 50% of reruns that are sky fibers

@sbailey
Copy link
Contributor Author

sbailey commented Nov 11, 2024

Good point. At the time of this PR I had not yet removed sky fibers from the reruns, so indeed many of the ZWARNs are for sky fibers. Additionally, low S/N spectra are more likely to have a Redrock / QuasarNet disagreement and also more likely to have a LOW_DELTA_CHI2 warning.

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.

4 participants