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

Generate spectra-64 per exposure and also create coadds not across cameras #945

Merged
merged 3 commits into from Apr 6, 2020

Conversation

akremin
Copy link
Member

@akremin akremin commented Apr 5, 2020

This addresses issue #914 .

Added lines to the bash script desi_nightly_redshifts to generate spectra-64 files for each exposure and create coadds across exposures that are not across cameras (in addition to across cameras).

This PR also fixes bugs in /scripts/coadd_spectra.py that removed exposures without all three cameras even when coadd-cameras was not set. The removal code now sits under that flag.

It also looks in input frames for the relevant cameras (bands) in frames2spectra instead of assuming all three are present. That is in pixgroup.py.

Files were tested in my SPECPROD using:

salloc -N 10 -t 1:00:00 -C haswell -q interactive
export SPECPROD=kremin
 desi_nightly_redshifts 20200314

outputs at nersc are in:
/global/cfs/cdirs/desi/spectro/redux/kremin/tiles/66000/20200314

…exposures but not across cameras in additional to across cameras
@akremin akremin changed the title Generate spectra-64 per exposure and create coadds not across cameras Generate spectra-64 per exposure and also create coadds not across cameras Apr 5, 2020
@sbailey
Copy link
Contributor

sbailey commented Apr 6, 2020

Added lines to the bash script desi_nightly_redshifts to generate spectra-64 files for each exposure and create coadds across exposures that are not across cameras (in addition to across cameras).

I suggest that we switch to only providing the "not across cameras" coadds and drop the across cameras version. In the cases of coadding across exposures but not across cameras, all the cameras should still be in the same output file, i.e. having the same format as the spectra files while in each HDU having only one row per target (coadds) instead of potentially multiple rows per targets (spectra files, with one row per individual exposure per target).

Regarding the spectra files: the "-64" part of the name originally referred to the healpix nside by which they were grouped (in addition to that info being in the headers), but that doesn't apply here since they are grouped by tile instead of healpix. At minimum let's drop the "-64" part of the name here.

It also appears that this makes one spectra file per exposure, when I think we want one spectra file per tile-spectrograph, same as the original coadd and redrock grouping. i.e. instead of "spectra-64-{expid}-{tileid}-{night}.fits" -> "spectra-{spectronum}-{tileid}-{night}.fits".

For consideration/discussion: This creates the potentially confusing situation that under the tiles/ directories the files would be called spectra-{spectronum}-{tileid}-{night}.fits grouped by tile-spectrograph, and for the main survey under the spectra-64/ directories they would be called spectra-{nside}-{healpix}.fits grouped by healpix. I think this is ok in the spirit that the spectra*.fits format is intended to be a generic format for holding an arbitrary grouping of spectra, regardless of how they were selected (by tile, by healpix, by target class, by visual inspection failure mode, whatever), so the "spectra" prefix implies a particular format rather than a particular grouping. But if people think that is going to cause an endless amount of confusion, we could call these "tilespec-*.fits" or something like that.

It looks like you had some other good catches in putting this together (dropped exposures, looking for frames instead of assuming), but let's converge on exactly what is grouped into files of what names before merging this.

@akremin
Copy link
Member Author

akremin commented Apr 6, 2020

All changes requested have been made.

In addition; the coadds and rrdesi are given the spectra-{spectro}-{tileid}-{night}.fits file as input instead of cframes and coadds respectively.

@sbailey
Copy link
Contributor

sbailey commented Apr 6, 2020

Looks good; thanks. Merging now.

@sbailey sbailey merged commit 343ba16 into master Apr 6, 2020
@sbailey sbailey deleted the coadd_features branch April 6, 2020 22:39
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

2 participants