desihub / tutorials

DESI tutorials
BSD 3-Clause "New" or "Revised" License
42 stars 16 forks source link

Update RedrockOutputs.ipynb #67

Closed RaffaellaCapasso closed 2 years ago

RaffaellaCapasso commented 2 years ago

This correction has to be made in order to have "ii" that are in the original order - otherwise the indices in "fm" and "zbest" do not correspond anymore and the later conditions on "goodELGs" are not correctly applied.

sbailey commented 2 years ago

Thanks @RaffaellaCapasso . @forero could you please double check this and merge if ready? It's unclear to me why the original notebook wasn't failing the next line assert np.all(fm['TARGETID'][ii] == zbest['TARGETID']) if sorting ii is necessary (this PR).

RaffaellaCapasso commented 2 years ago

@sbailey I believe it's because assert here cycles through fm['TARGETID'][ii] and checks for matches in any zbest['TARGETID'] - the indices here don't have to be in the same order. The problem arises if we want to add conditions on goodELGs using fm, e.g. goodELGs &= (fm['DESI_TARGET'] != 0) or goodELGs &= (fm['OBJTYPE'] == 'TGT'), while having conditions using zbest, e.g. goodELGs &= (zbest['DELTACHI2']>25). In this case, it matters that the indices in zbest and fm coincide.

RaffaellaCapasso commented 2 years ago

Also, it might be better change these two lines as well, if one wants to use it on fuji (or on data, in general):

specfile = desispec.io.findfile('spectra', groupname='5299', nside=64) zbestfile = desispec.io.findfile('zbest', groupname='5299', nside=64)

Not sure what the best way to do this is, in my code I just specified the fits files instead, e.g.:

specfile = "global/cfs/cdirs/desi/spectro/redux/fuji/tiles/cumulative/132/20210414/coadd-0-132-thru20210414.fits" zbestfile = "global/cfs/cdirs/desi/spectro/redux/fuji/tiles/cumulative/132/20210414/redrock-0-132-thru20210414.fits"

spectra = desispec.io.read_spectra(specfile)
zbest = Table.read(zbestfile, hdu=1)

forero commented 2 years ago

Thanks @RaffaellaCapasso . Unfortunately, I cannot get the updated notebook to run. It fails at the assert mentioned by @sbailey in the same cell.

RaffaellaCapasso commented 2 years ago

Hi @forero, thanks for looking into this! What error do you get? I don't get errors by using any data (right now testing it with fuji, but previous datasets also worked).

forero commented 2 years ago

Hi @RaffaellaCapasso . This is what I am getting

---------------------------------------------------------------------------
AssertionError                            Traceback (most recent call last)
<ipython-input-5-27e474c0c029> in <module>
      1 fm = Table.read(zbestfile, hdu='FIBERMAP')
      2 ii=sorted(np.unique(fm['TARGETID'], return_index=True)[1])
----> 3 assert np.all(fm['TARGETID'][ii] == zbest['TARGETID'])
      4 fm = fm[ii]

AssertionError: 

I am using the data originally referenced in the notebook: /global/cfs/cdirs/desi/datachallenge/reference_runs/20.8

RaffaellaCapasso commented 2 years ago

I made all the changes to update the tutorial to work with fuji data, on kernel 22.2. Please double check this and merge if everything looks good.

forero commented 2 years ago

@RaffaellaCapasso I just tested the notebooks and made a couple of small modifications (including updating the README). This looks good to me, so I am merging now. If someone finds problems with these notebooks, let's deal with them on a separate issue/PR.

Thanks a lot for your contribution!