desihub / desitarget

DESI Targeting
BSD 3-Clause "New" or "Revised" License
18 stars 23 forks source link

Refactor data model and I/O for the QSO redshift catalogs #737

Closed geordie666 closed 3 years ago

geordie666 commented 3 years ago

This PR tweaks the data model and directory structures when making the "zqso" redshift catalog files for Lyman-alpha repeat decisions.

geordie666 commented 3 years ago

@sbailey: This should be (I hope) all that is needed to run the code to produce the zqso files for the MTL repeats, to accompany the zbest files. I think the instructions are simple:

Just in case, you may also want to set up SQUEzE in a similar manner:

git clone https://github.com/iprafols/SQUEzE
export PYTHONPATH=/where/I/put/it/git/SQUEzE/py:$PYTHONPATH
setenv SQ_MODEL_FILE $env(DESI_ROOT)/target/catalogs/lya/sq_models/BOSS_train_64plates_model.json
geordie666 commented 3 years ago

@kirkby, @dylanagreen: Could I ask you to:

  1. Take a look at the zqso files I've made in /global/cscratch1/sd/adamyers/zqsotest and confirm that the outputs for the Z_QN, Z_QN_CONF, IS_QSO_QN columns look sensible?
  2. Tag https://github.com/desihub/QuasarNP? Or greenlight myself or @sbailey to cut a tag?
coveralls commented 3 years ago

Coverage Status

Coverage decreased (-0.3%) to 59.072% when pulling c6e168712a5c82feec181e8213073600ec3108c3 on ADM-MTL-LyA into 9faa019ef6c3fac127356589d68288caeaf74c7d on master.

geordie666 commented 3 years ago

As noted on the recent telecon, I will leave this open while I clean up the outputs to only include columns directly used in MTL decisions.

geordie666 commented 3 years ago

I'm done with this now, I think. I've added all of the extra redrock columns requested on the recent telecon, and removed placeholder columns for unused afterburners.

I'll merge this in about 1.5 hours to keep making progress with the end-to-end MTL loop, which will incorporate the zqso catalogs from this branch.

sbailey commented 3 years ago

One request: the code currently requires squeze to be installed even if it isn't using it. Let's move the lyazcat.py "from squeze.model import Model" etc lines into the functions that use them so that they are only imported if needed, or otherwise wrap them in a try/except ImportError block so that it isn't fatal if squeze isn't installed. We could consider the same for quasarnp imports, but that seems less critical since QuasarNP is baselined as something we will use by default and need to have installed (but good that it still isn't required for running other pieces of desitarget).

I tried the examples and they work as advertised, thanks.

geordie666 commented 3 years ago

OK, I'll do this before merging. And, I'll shift quasarnp imports in a similar way. I think the code should either expect one, or both.

geordie666 commented 3 years ago

I've updated the code so that SQUEzE is only a requirement if the --add_squeze option (or associated functions) are actually used. In the end, I didn't mirror this choice for QuasarNP as it slowed the code down when looping through multiple tile/night/petal combinations. So, QuasarNP remains an explicit requirement of running any of the code.

I'll merge once tests pass.

dylanagreen commented 3 years ago
1. Take a look at the `zqso` files I've made in `/global/cscratch1/sd/adamyers/zqsotest` and confirm that the outputs for the `Z_QN, Z_QN_CONF, IS_QSO_QN` columns look sensible?

2. Tag `https://github.com/desihub/QuasarNP`? Or greenlight myself or @sbailey to cut a tag?

@geordie666 I have just finished adding docstrings to QuasarNP, and have tagged it at version 0.1.0 . The snippet of the zqso file that you showed on the telecon looked reasonable, although I haven't gone as in depth as to read the entire file to double check.

geordie666 commented 3 years ago

Thanks, @dylanagreen. I spot-checked a number of the output files, and find that the redshifts agree between redrock and QN for ~75% of cases. So, I think the files look good, barring some really perverse bug.

sbailey commented 3 years ago

@geordie666 @dylanagreen who do think should "own" setting the default $QN_MODEL_FILE ? i.e. should "module load QuasarNP/0.1.0" set that, or should QuasarNP be agnostic about what model file will be used and let desitarget set that when its module file is loaded? Either way, a user can override that for testing, but where should we set the default?

Minor: desitarget currently has a defined module file in desitarget/etc/desitarget.module, while QuasarNP does not. We could add that, but it gets a bit more of desi-specific hooks into an otherwise mostly experiment-agnostic package.

geordie666 commented 3 years ago

I think desitarget should own the $QN_MODEL_FILE. That file is a default for DESI targeting, not necessarily for all possible uses of QuasarNP.

It's entirely possible that we don't change anything about the QuasarNP code but we do change the $QN_MODEL_FILE, i.e. if we retrain the QN model using DESI data instead of SDSS data.

sbailey commented 3 years ago

Thanks @geordie666, I agree with your reasoning. I now post-facto see that you already added QN_MODEL_FILE and SQ_MODEL_FILE to the etc/desitarget.module file. Good!