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

restore template-generating code to a working state #556

Merged
merged 17 commits into from Jan 8, 2022

Conversation

moustakas
Copy link
Member

desisim.templates has been broken for some time (e.g., #553). The changes in this PR are needed to restore everything to a working state.

WIP so not quite ready to merge.

The companion PRs are imcgreer/simqso#33 and desihub/desitarget#786. However, I'm having trouble getting the Github actions tests to pass even though they pass on my laptop. I'll keep trying.

One of the "major" changes is that desisim.templates now has to estimate the fiberflux for the ELG and LRG target classes, in order to retrieve templates / models which pass our nominal color-cuts. Since the template-generating code doesn't know much physics, I opted to generate these fluxes statistically, using data from SV3.

Here's my code, for the record:

import numpy as np
from astropy.table import Table
from desitarget.sv3.sv3_targetmask import desi_mask

cd, '/global/cfs/cdirs/desi/spectro/fastspecfit/everest-v1/catalogs'
darkmeta = Table.read('fastspec-everest-sv3-dark.fits', 'METADATA')
brightmeta = Table.read('fastspec-everest-sv3-bright.fits', 'METADATA')

for targ, zminmax, meta in zip( ('ELG', 'LRG', 'BGS_ANY'), ((0.6, 1.6), (0.3, 1.0), 
  (0.05, 0.5)), (darkmeta, darkmeta, brightmeta) ):
    indx = np.where((meta['SV3_DESI_TARGET'] & desi_mask[targ] != 0) * 
      (meta['Z'] > zminmax[0]) * (meta['Z'] < zminmax[1]) * (meta['ZWARN'] == 0) * 
      (meta['FLUX_G'] > 0) * (meta['FIBERFLUX_G'] > 0) * 
      (meta['FLUX_R'] > 0) * (meta['FIBERFLUX_R'] > 0) * 
      (meta['FLUX_Z'] > 0) * (meta['FIBERFLUX_Z'] > 0))[0] 
    print('Selected {:,} {}'.format(len(indx), targ))

    for band in ('G', 'R', 'Z'):
      ratio = meta['FIBERFLUX_{}'.format(band)][indx] / meta['FLUX_{}'.format(band)][indx]
      print('  Median, Mean, Sigma {} fiberflux / flux = {:.4f}, {:.4f}, {:.4f}'.format(
        band, np.median(ratio), np.mean(ratio), np.std(ratio)))

Output:

Selected 270,630 ELG
  Median, Mean, Sigma G fiberflux / flux = 0.6026, 0.5901, 0.1275
  Median, Mean, Sigma R fiberflux / flux = 0.6171, 0.6037, 0.1292
  Median, Mean, Sigma Z fiberflux / flux = 0.6307, 0.6164, 0.1311
Selected 115,565 LRG
  Median, Mean, Sigma G fiberflux / flux = 0.3992, 0.4096, 0.1366
  Median, Mean, Sigma R fiberflux / flux = 0.4087, 0.4191, 0.1392
  Median, Mean, Sigma Z fiberflux / flux = 0.4175, 0.4280, 0.1417
Selected 241,779 BGS_ANY
  Median, Mean, Sigma G fiberflux / flux = 0.3025, 0.3085, 0.1261
  Median, Mean, Sigma R fiberflux / flux = 0.3097, 0.3158, 0.1286
  Median, Mean, Sigma Z fiberflux / flux = 0.3165, 0.3225, 0.1311

@weaverba137
Copy link
Member

Starting to look at the test suite. Has the simqso branch astropy-updates been merged and tagged?

@coveralls
Copy link

coveralls commented Jan 7, 2022

Coverage Status

Coverage decreased (-0.005%) to 45.386% when pulling 898d593 on templates-fiberflux into 8526bfd on master.

@moustakas
Copy link
Member Author

Starting to look at the test suite. Has the simqso branch astropy-updates been merged and tagged?

Unfortunately, no. Here's the PR--
imcgreer/simqso#33

I wonder whether we should fork simqso into desihub so we can make changes as needed. @sbailey, thoughts?

@weaverba137
Copy link
Member

The fitsio install trick is helping but it is not the full story here. In the near future the specsim package will need to be updated to work with astropy 5.

After temporarily disabling tests on astropy 5, the one remaining error is #507, #549. It is well past time that got some attention.

Finally, I notice there is a test, test_wd_subtype_failure in test_templates.py that is marked as an expected failure. Is that because this will never be fixed? If it will never be fixed, why not refactor the WD code so that this failure mode is avoided (e.g. WDs don't have subtypes in desisim)?

@moustakas
Copy link
Member Author

Thanks @weaverba137. I propose we merge this PR and I'll tackle #507 and #549 separately. For the record, this issue has gotten lots of attention over the years but the issue has been annoyingly tricky to figure out for good. But I'll give it my best shot (again)!

Any objections to forking the simqso package into desihub?

Finally, I notice there is a test, test_wd_subtype_failure in test_templates.py that is marked as an expected failure. Is that because this will never be fixed? If it will never be fixed, why not refactor the WD code so that this failure mode is avoided (e.g. WDs don't have subtypes in desisim)?

I wrote that unit test years ago and was probably trying to be clever. I'll take a look in a new branch.

@weaverba137
Copy link
Member

Let's move the discussion of simqso to a separate thread. Go ahead & merge.

@moustakas moustakas merged commit 296bf7a into master Jan 8, 2022
@moustakas moustakas deleted the templates-fiberflux branch January 8, 2022 01:06
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