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

fix qproc non-empty fibermaps #1587

Merged
merged 5 commits into from
Jan 14, 2022
Merged

fix qproc non-empty fibermaps #1587

merged 5 commits into from
Jan 14, 2022

Conversation

sybenzvi
Copy link
Contributor

Two updates needed for using non-empty fibermaps in qproc:

  1. Pass fibermap to read_raw to prevent preproc from creating an empty default fibermap.
  2. Select fibermap rows corresponding to one camera to prevent qproc_boxcar_extraction from running past the end of the input traceset.

These changes address #1315. I tried not to touch qproc to fix the issue but the changes are necessary.

@sybenzvi sybenzvi marked this pull request as draft January 12, 2022 15:30
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.006%) to 25.198% when pulling 351eb94 on fibermap_fix_jan22 into 8a8bd59 on master.

@sybenzvi sybenzvi marked this pull request as ready for review January 12, 2022 16:36
@sbailey
Copy link
Contributor

sbailey commented Jan 12, 2022

This looks fine to me, but for a cross check could you provide a cut-and-pasteable test case that fails (or does the incorrect thing) with current master and works with this branch?

@sybenzvi
Copy link
Contributor Author

This looks fine to me, but for a cross check could you provide a cut-and-pasteable test case that fails (or does the incorrect thing) with current master and works with this branch?

Yes, I'll add a unit test that demonstrates that not including the fibermap just reproduces the behavior in the main branch.

@sbailey
Copy link
Contributor

sbailey commented Jan 12, 2022

I don't want to turn down the offer for adding unit tests, but I was asking for something even more minimal than that: what nightwatch or qproc command should someone run and how can they tell whether the right fibermap is being used, even if done as a one-off test of the PR?

@sybenzvi
Copy link
Contributor Author

sybenzvi commented Jan 12, 2022

OK, in the course of writing instructions on how to test this, I discovered a problem with the branch, so I'm converting the PR to a draft until I fix it.

Meanwhile, here are instructions for a one-off test at NERSC. This default mode of creating an empty fibermap in preproc and using that empty fibermap in qproc is tested by running:

desi_qproc -i /global/project/projectdirs/desi/spectro/data/20220111/00118039/desi-00118039.fits.fz --camera z1

To test using a fibermap, create one and use it as follows:

assemble_fibermap -n 20220111 -e 118039 -o fibermap-00118039.fits
desi_qproc -i /global/project/projectdirs/desi/spectro/data/20220111/00118039/desi-00118039.fits.fz --fibermap fibermap-00118039.fits --camera z1 --auto

Then check the FIBERMAP HDU in the preproc and qproc output FITS files using desispec.io.read_fibermap. Here is a script check_fibermap.py to do that:

from argparse import ArgumentParser
from desispec.io import read_fibermap

p = ArgumentParser(description='Read and dump fibermap to stdout')
p.add_argument('ifiles', nargs='+', help='Input fits file with a FIBERMAP HDU')

args = p.parse_args()

for ifile in args.ifiles:
    print(f'\n{ifile}')
    try:
        fibermap = read_fibermap(ifile)
        print(fibermap)
    except Exception as e:
        print(e)

Please let me know if you want this documented in a README or if this comment is good enough.

@sybenzvi sybenzvi marked this pull request as draft January 12, 2022 21:24
@sbailey
Copy link
Contributor

sbailey commented Jan 12, 2022

This comment is good enough; no need for adding README. Thanks for specific example.

@sybenzvi
Copy link
Contributor Author

I'm converting this PR back from draft mode. The issue was I noticed differences in the Nightwatch QA and wanted to track down the origin. I confirmed the PR does not affect any of the qframe or qcframe output. Only qsky is different, because if a good fibermap with SKY fibers isn't available, qproc sorts the fibers by flux and uses the 50% faintest ones to compute the background.

@sybenzvi sybenzvi marked this pull request as ready for review January 12, 2022 22:43
@sbailey sbailey merged commit fe02f7b into master Jan 14, 2022
@sbailey sbailey deleted the fibermap_fix_jan22 branch January 14, 2022 00:42
@sbailey sbailey changed the title Fibermap fix jan22 fix qproc non-empty fibermaps Jan 17, 2022
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.

3 participants