Add the final QSO decision logic to the full Main Survey MTL loop.

[geordie666]

This PR should be the final piece needed for the full MTL loop for the Main Survey. It adds the logic for deciding which quasars are observed at high priority on repeat passes. To try to succinctly describe the logic:

• Let Z denote the redrock redshift.
• Let Z_QN denote the QuasarNP redshift.
• Let IS_QSO_QN denote the QuasarNP decision as to whether something is a quasar or not.

Then:

• If a QSO target has Z > 2.1 OR (Z_QN > 2.1 AND IS_QSO_QN==1) it is confirmed as a LyA quasar. It receives 3 more observations (for a total of 4 observations) at a very high priority (just below an unoberved quasar).
• If a QSO target has Z < 1.6 OR Z_QN < 1.6 OR IS_QSO_QN !=1 it is confirmed as a (possible) tracer quasar. It receives 1 more observation (for a total of 2 observations) at a priority lower than all other targets but above "filler" targets.
• All other QSO targets are considered to be "mid-z" quasars (requested by a secondary program). They receive 3 more observations (for a total of 4 observations) at a priority lower than all other targets but above "filler" targets.

Additionally:

• LyA QSOs are "locked in" to their priority state as soon as they are determined to be a LyA quasar. This is to ameliorate a bout of unlucky spectroscopy consigning them to be tracer quasars from which it may be difficult to recover.
• All other quasar targets are allowed to be promoted to LyA quasars should they meet the LyA criteria on subsequent passes. No quasar target ever receives more than 4 total observations, though.
• "Standalone" secondary QSO programs (WISE_VAR_QSO, QSO_RED) share the same logic as for primary QSOs. Almost all (standalone) targets in these programs fall in the tracer category, though. This is generally because of the shallower WISE selection(s) used by these programs, which skews the redshift distribution to z~1 compared to quasars selected with more overall weight given to optical data.

I'll use this branch to make MTL ledgers for review once the zqso catalogs are ready at NERSC. I'll subsequently merge this PR if those MTL ledgers look reasonable to the Operations team and some select quasologists.

[geordie666]

The basic/main QSO cases (tracer/secondary-mid-z-QSO/LyA-QSO) are already in the unit tests. There are possibly one or two that I've missed, which I can add relatively quickly.

Adding "exhaustive" tests will require significant new functionality that we have never previously included. First, we would also need to check decisions on passes 2, 3 and 4 of the overlapping observations, not just on passes 0 and 1. Second, we would also need to include secondary targets to be truly exhaustive.

There are potentially dozens of individual cases as a few of the secondary targets request additional observations and could, in theory, merge with a primary QSO that takes precedence in dark time but not in bright time. Checking many of these cases using real data took me several days of work for this PR. Writing such tests with ersatz data will potentially take a great deal of time and delay merging this PR (and the creation of the subsequent MTL catalogs). I doubt I can add all of the cases by Monday.

We also don't have tests that check the full MTL loop, and never have. To some extent, I see checking pass-2/3/4 decisions and looking for corner cases as the purview of mocks rather than of unit tests. We have never unit-tested beyond passes 0 and 1.

[geordie666]

I added unit tests to:

• Check dark-time targets behave as expected after 0, 1, 2, 3, 4 passes, in some basic sense.
• Check quasar targets are "locked into" their priority state once they become a LyA QSO.

If I find some time over the weekend I'll add a few more cases.

[geordie666]

I extended the "locked in" test to an additional pass, and added a new unit test, to check that:

• A mid-z QSO promoted to being a LyA QSO on the second pass retains that status for the third pass, even if it's redshift reverts to mid-z.
• Mid-z QSOs are not "locked in," i.e. they can revert to being tracer QSOs as soon as a redshift combination is measured that suggests they might be tracers.

[geordie666]

I extended the "locked in" test further, to check that:

• A tracer QSO can also be promoted to being a LyA QSO, and is then "locked in" to being a LyA QSO.
• Regardless of updating redshifts in different ways on different passes, every QSO is still DONE after a full 4 passes.

[sbailey]

Thanks for adding the extra tests. Can you confirm that the LyA followup choices purposefully do not use ZWARN or DELTACHI2? I don't see that in the description, which surprises me, especially for more dramatic ZWARN bits like NODATA, Z_FITLIMIT, or BAD_MINFIT. Or is that caught earlier before a target is even considered for LyA followup?

Update: I now see the docstring comment about "Sources in the zcat with ZWARN of NODATA are always ignored". Please confirm if any other ZWARN bits or DELTACHI2 are used (or purposefully ignored).

[sbailey]

Running make_mtl alters the input zcat table by adding a NUMOBS_MORE column. Ideally make_mtl should not alter the inputs, or otherwise this side effect should be documented (including why). If you are only adding a column for bookkeeping convenience but not otherwise modifying the rows of the pre-existing columns, you could start with

zcat = zcat.copy(copy_data=False)

[sbailey]

@geordie666 are your criteria applied in order and the first matching one determines the state? e.g.

Z=0.5, QN_Z=2.5, IS_QSO_QN==1 matches your first criterion as a LyA quasar but matches your second criterion as a tracer quasar. Does the first criterion win?

[apologies to others getting N>>1 emails via this ticket from me; more are likely coming...]

[sbailey]

minor: make_mtl docstring says that targets requires columns TARGETID, DESI_TARGET, NUMOBS_INIT, PRIORITY_INIT; experimentally it also requires BGS_TARGET, MWS_TARGET, and if zcat is not None then it also requires PRIORITY (i.e. if zcat is not None then targets needs to be a previous mtl, not an original targets table, even if this is the first iteration). That matches how targets -> mtl ledger -> obs -> mtl updates happens in practice, but is somewhat inconsistent with the docstring.

[sbailey]

@geordie666 FWIW I independently tested all the initial cases of RR/QN for the first MTL update. Initially I found a bunch of errors, then realized that I had the master branch checked out; switching to this branch fixed all the failing cases. Good! Attached is a gzipped python script I used (GitHub doesn't allow direct .py attachments) in case it covers any cases that you don't think you have already covered in unit tests.

check_lya_mtl.py.gz

I have not independently tested the "locked in" and "promoted" cases, though it sounds like you have those included in your unit tests.

Summarizing my tests/questions for the night:

• Please confirm that make_mtl purposefully does not use DELTACHI2, and NODATA is the only ZWARN bit used
• make_mtl alters zcat, which is undesireable
• Docstring comments
• I have independently tested the functionality except for the LyA lockin and promotion, and didn't find any problems with this branch

[geordie666]

@sbailey: In answer to your comments:

• For the Main Survey, make_mtl() purposefully does not use DELTACHI2, and NODATA is the only ZWARN bit used. But for SV, both DELTACHI2 and ZWARN were needed and used, so are retained throughout for backwards compatibility.
• make_mtl( ) has added a NUMOBS_MORE column to the zcat for ~5 years, I think?

So, I'm not sure what your request is, here? If the issue is that the number of rows in the zcat can potentially change, that's been possible since you updated the code back in late 2018 (specifically, your zcat = zcat[ok] line can alter the number of rows in the zcat):

My only change in this initial zcat-altering-behavior was to log that this alteration was a possibility. Essentially, I don't think this specific PR changes existing allowed behavior in any way, it merely inherits behavior from earlier PRs?

• Ah, yes, I updated the docstrings in calc_numobs_more and calc_priority but not in make_mtl. I'll update that before I merge this PR. Thanks!
• It's great that you've independently tested the functionality. To answer one more of your questions: Z=0.5, QN_Z=2.5, IS_QSO_QN==1 matches your first criterion as a LyA quasar but matches your second criterion as a tracer quasar. Does the first criterion win?. I should have specified that the tracer/mid-z QSOs are only ever drawn from the population of quasars that are explicitly not LyA quasars. So, yes, the first (LyA) criterion wins.

[geordie666]

Sorry, I misunderstood your 2nd point, above, by conflating a discussion of the MTL table with a discussion of the zcat. I think I see what you mean, now.

Although the zcat has been modified-in-place by make_mtl() for as long as I can remember, nothing about processing the "real" data requires that modification, so I've always assumed it was a necessary feature for the mocks. Can you confirm that the mocks will be just fine if I force the output zcat from make_mtl() to always be identical to the input zcat?

[sbailey]

Thanks for the additional info @geordie666 . I'm apparently 5 years late in noticing that make_mtl modifies zcat by adding a new NUMOBS_MORE column. I suspect that is an unintended side-effect of the bookkeeping rather than something actually used for mocks, but in the interest of not breaking things at the last moment let's just document this behavior while we're noticing it, but not change it right now. i.e. this is a technical debt/maintenance issue to document whenever a function modifies its inputs (as opposed to just using them to derive outputs).

Additionally, if you know of any other inputs that are modified, or know whether make_mtl would use a pre-existing NUMOBS_MORE column, that should be documented too.

Otherwise I'm satisfied with the functionality of this PR. Thanks.

[geordie666]

I expect that this is ready to merge, now, but I'm still going to run a few more tests through the rest of the evening. I'll likely self-merge this early tomorrow.

[coveralls]

Coverage decreased (-0.005%) to 58.761% when pulling dceb6d4434e9e8dc01ec380d11e976d7277e40a5 on ADM-LyA-logic into cb8812282dd57b530b81d5e7556e683bcd66a84d on master.