Add a new wrapper to create all of the zcatalogs with a single command

akremin commented 2 weeks ago

Overview

This creates a new script, currently called desi_zcatalog_wrapper, that will use the tiles-SPECPROD.fits file in a production to identify which SURVEY+PROGRAM pairs exist and produce zcatalogs for each of them. If all are created successfully, then the zall-* catalog is created with all of the individual zcatalogs.

Required arguments are: --groupto specify whether you're running healpix redshifts or cumulative redshifts. It should also work for other tile-based flavors but I have not tested that. --cat-version OR --outdir to specify where to save the zcatalogs. If --cat-version is specified then it is saved under $DESI_SPECTRO_REDUX/$SPECPROD/zcatalog/<cat_version>/.

The user can specify --indir, but it is optional and defaults to the typical places for a production.

All of the boolean flags accepted by desi_zcatalog are also accepted by this script, which propagates them to desi_zcatalog. Though I did not thoroughly vet my implementation of these.

This PR also includes updates to desispec.io.meta.findfile to include the ztile, zpix, and zall files. That required adding a new function argument version that is only used for these redshift catalogs at the moment but could be used for other things in the future. Because there is ongoing work on the zcatalogs v2, I did not edit the desi_zcatalog code itself to use findfile, but that should be updated to use findfile at some point during the v2 refactor.

Tests

Recommended usage, which uses only --cat-version and --group:

desi_zcatalog_wrapper -g healpix --cat-version=v1 --nproc=2 -v desi_zcatalog_wrapper -g cumulative --cat-version=v1 --nproc=2 -v

Alternate method that uses --outdir

desi_zcatalog_wrapper -g healpix --outdir=/global/cfs/cdirs/desi/users/kremin/PRs/zcat_maker/test --nproc=2 desi_zcatalog_wrapper -g cumulative --outdir=/global/cfs/cdirs/desi/users/kremin/PRs/zcat_maker/test --nproc=2

Alternate method that uses cat-version for output but specify input

desi_zcatalog_wrapper -g cumulative --cat-version=v2 --nproc=2 -i /global/cfs/cdirs/desi/spectro/redux/loa/tiles/cumulative desi_zcatalog_wrapper -g healpix --cat-version=v2 --nproc=2 -i /global/cfs/cdirs/desi/spectro/redux/loa/healpix

Alternate method that uses --indir and --outdir

desi_zcatalog_wrapper -g healpix -i /global/cfs/cdirs/desi/spectro/redux/loa/healpix -outdir=/global/cfs/cdirs/desi/users/kremin/PRs/zcat_maker/test2 --nproc=2 desi_zcatalog_wrapper -g cumulative -i /global/cfs/cdirs/desi/spectro/redux/loa/tiles/cumulative --outdir=/global/cfs/cdirs/desi/users/kremin/PRs/zcat_maker/test2 --nproc=2

These all work. I also linked the zcatalogs from loa and ran again, and the zall generation appears to work as expected.

coveralls commented 2 weeks ago

coverage: 30.078% (-0.07%) from 30.145% when pulling 5e3f3cbd285db8d781bcf39802281a1186c0c952 on zcat_maker into 08d4acd9fee58a28bf4d08782083f9bfbd6e502d on main.

akremin commented 2 weeks ago

The doc generation passes on my checkout at NERSC and there is nothing informative I have been able to glean from the GitHub run logs.

sbailey commented 6 days ago

Looking at /global/cfs/cdirs/desi/users/kremin/PRs/zcat_maker/test/zcatalog/v1/zpix-cmx-other.fits, it appears that this didn't add units. I made the same mistake when running Loa by hand (thanks to @weaverba137 for noticing; I'll post-facto fix that), but let's get this wrapper updated so that it automatically includes units too.

akremin commented 6 days ago

I had assumed this was fixed downstream since https://github.com/desihub/desispec/issues/2364 was closed. The wrapper includes the --add-units option, just as desi_zcatalog does.

Given that the issue isn't resolved, I am happy to fix it in this PR. I can think of two options:

1) Remove the --add-unit option for both the wrapper and desi_zcatalog and always add units. 2) Flip the option to be opt-out, i.e. --do-not-add-units for both the wrapper and desi_zcatalog

Also happy to hear other ideas

weaverba137 commented 6 days ago

@akremin, I know this is somewhat deeply buried, but in order for --add-unit to do anything at all, module load desidatamodel must be run before the script starts.

desihub / desispec