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

add QA+zmtl support #1310

Merged
merged 6 commits into from Jun 18, 2021
Merged

add QA+zmtl support #1310

merged 6 commits into from Jun 18, 2021

Conversation

sbailey
Copy link
Contributor

@sbailey sbailey commented Jun 16, 2021

This PR adds support for combining tile_qa with zmtl creation (formerly zqso, owned by desitarget now ported to desispec). This PR requires desihub/desitarget#754 (branch zwarn_badspecqa) to get two additional ZWARN bit definitions for BAD_SPECQA and BAD_PETALQA.

Example outputs are in /global/cfs/cdirs/desi/users/sjbailey/spectro/redux/zmtl , see README with details e.g.

[setup the prod, then...]
desi_tile_qa -n 20210606 -t 1230,1246,1394
make_zmtl_files -n 20210606 -t 1230
make_zmtl_files -n 20210606 -t 1246
make_zmtl_files -n 20210606 -t 1394

I added updates for running this as part of the pipeline but haven't tested that yet.

Let's also re-check the QA before merging. e.g. https://data.desi.lbl.gov/desi/users/sjbailey/spectro/redux/zmtl/tiles/cumulative/1394/20210606/tile-qa-1394-thru20210606.png flags THRUFRAC_X in red, but unexpected went from 178 good fibers (below threshold) to 257 good fibers (above threshold) compared to @julienguy original run at https://data.desi.lbl.gov/desi/users/jguy/exposure_qa/tiles/cumulative/1394/20210606/tile-qa-1394-20210606.png that rejected the entire tile.

This PR does not:

  1. run desi_exposure_qa automatically at the time of cframes and propagate QAFIBERMASK forward into spectra/coadd/zbest FIBERMAP.FIBERSTATUS (it only gets run retroactively by desi_tile_qa at the end)
  2. set FIBERSTATUS bit POORPOSITION at the start along side BADPOSITION, independent of the QA.
  3. do anything with how tiles.csv is updated.

I'd like to do those soon, but they aren't strictly required for the initial round of zmtl updates for unlocking tiles.

@julienguy @akremin @geordie666

@sbailey
Copy link
Contributor Author

sbailey commented Jun 16, 2021

Update: I just merged desitarget PR #754 and updated it at NERSC, so this branch can now be tested with desitarget/master .

@julienguy
Copy link
Contributor

One comment possible for a future PR:
function zmtl.add_abs_data is the MgII line fitter :
The code would gain clarity and be easier to maintain if this line fitter was separated from zmtl and put in an other function in another python file. The inputs should be made more explicit (the whole zmtl catalog is probably not needed).

@geordie666
Copy link
Contributor

I'd be fine with pruning all of the functions that aren't used (such as SQUEzE and Lucas Napolitano's MgII absorption fitter). There'll always be a copy of those in old branches of desitarget if we want to reincorporate them into wider quasar catalogs as afterburners.

I'm also totally OK with leaving everything exactly as-is for expediency.

@sbailey
Copy link
Contributor Author

sbailey commented Jun 18, 2021

For expediency, I will leave the optional MgII and SQ code in place until we have converged on moving them elsewhere.

It appears that github actions are broken right now (tests are being canceled without any log output), but I have verified that tests pass at NERSC (including building docs).

@sbailey sbailey merged commit a17f043 into master Jun 18, 2021
@sbailey sbailey deleted the zmtl branch June 18, 2021 18:02
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