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

Improvement to sky subtraction with a PCA #1381

Merged
merged 23 commits into from Aug 26, 2021
Merged

Improvement to sky subtraction with a PCA #1381

merged 23 commits into from Aug 26, 2021

Conversation

julienguy
Copy link
Contributor

In this PR is implemented a principal component analysis of the wavelength and line spread function corrections to the emission lines in the sky spectra. The PCA components are based the analysis of many dark time exposures. For each science exposure, a linear combination of the components is fit using the measured corrections to the sky lines of the sky fibers only, and then the corrections are applied to all fibers. Compared to the Everest data release, this method reduces the residual scatter on sky lines by 10 to 20% depending on the camera.
See more details and plots in https://desi.lbl.gov/DocDB/cgi-bin/private/ShowDocument?docid=6437 .

Changes to the code

  • Changes to desi_compute_sky
    • add option --adjust-with-more-fibers use more fibers than just the sky fibers for the adjustments of the sky line wavelength and width (used only to build the PCA components)
    • add option --save-adjustments SAVE_ADJUSTMENTS save adjustments of wavelength calibration and LSF width in fits images (fibers x wavelength) (used only to build the PCA components)
    • add option --pca-corr PCA_CORR use this PCA frames file for interpolation of wavelength and/or LSF adjustment (used by default for sky subtraction in desi_proc)
  • Changes to desi_proc
    • add options --adjust-sky-with-more-fibers and --save-sky-adjustments, only used to build the PCA components, which saves the adjustments as fits files 'skycorr-camera-expid.fits' is the reduced exposure directory.
    • uses by default desi_compute_sky with option --pca-corr FILENAME , where the FILENAME is found with the calibfinder.
  • New script desi_compute_skycorr_pca to compute the PCA components from a large list of 'skycorr*.fits' files.

Changes to the calibration SVN

  • Add files $DESI_SPECTRO_CALIB/spec/sm*/skycorr-pca-sm*-*.fits (one per camera)
  • Add keyword SKYCORR spec/sm*/skycorr-pca-sm*-*.fits in the yaml files.

@coveralls
Copy link

coveralls commented Aug 21, 2021

Coverage Status

Coverage increased (+0.2%) to 26.819% when pulling a810249 on skysub into 2777a0d on master.

Copy link
Contributor

@sbailey sbailey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good though I'd like to poke at running it a bit more after cori is back, before merging. I put some maintenance/maintainability comments inline for consideration.

In desi_spectro_calib, the skycorr files are now the biggest thing in the spec subdirectory. Let's change those to single precision float to save 2x in size. gzipping doesn't help much beyond that.

# and then apply the linear combination to all fibers
log.info("Reading {}".format(pca_corr))

hdulist = pyfits.open(pca_corr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks our usual rule of separating I/O from algorithms. Consider reading that in with a desispec.io.read_skycorr routine and passing the correction model into compute_sky -> compute_uniform_sky .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added I/O

py/desispec/sky.py Outdated Show resolved Hide resolved
py/desispec/sky.py Show resolved Hide resolved
@julienguy
Copy link
Contributor Author

julienguy commented Aug 25, 2021

I/O tested in isolation but not in production

@julienguy
Copy link
Contributor Author

Changes tested in small prod with desi_proc. All comments so far have been addressed. Unless there are more, this PR could be merged once the impact on redshifts is evaluated.

@sbailey
Copy link
Contributor

sbailey commented Aug 26, 2021

Discussed with Julien and we won't wait for redshift tests on this PR specifically, but rather bundle those tests with other recent algorithmic updates, so I'll merge this now.

@sbailey sbailey merged commit 7e8b39f into master Aug 26, 2021
@sbailey sbailey deleted the skysub branch August 26, 2021 19:47
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

3 participants