desihub / desisim

DESI simulations
BSD 3-Clause "New" or "Revised" License
16 stars 22 forks source link

Suspiciously low QSO flux #507

Closed weaverba137 closed 2 years ago

weaverba137 commented 5 years ago

Independently of the Numpy 1.17 issue, we are also seeing this error on some but not all tests:

======================================================================

FAIL: test_newexp (desisim.test.test_obs.TestObs)

----------------------------------------------------------------------

Traceback (most recent call last):

  File "/home/travis/build/desihub/desisim/py/desisim/test/test_obs.py", line 103, in test_newexp

    self.assertTrue(maxflux > 0.01, 'suspiciously low {} flux ({}) using seed {}; wrong units?'.format(objtype, maxflux, seed))

AssertionError: False is not true : suspiciously low QSO flux (0.0) using seed 800364; wrong units?

----------------------------------------------------------------------

It appears that some seeds are just "bad luck" and trigger this bug.

moustakas commented 4 years ago

Using the seed from this test failure, I'm able to reproduce the problem:

import numpy as np
from desisim.templates import QSO
qq = QSO()
ff, ww, mm, oo = qq.make_templates(1, seed=458870270)
WARNING:templates.py:2258:make_templates: Maximum number of iterations reached on QSO 0, z=1.47216.
WARNING:templates.py:2264:make_templates: 1 spectra could not be computed given the input priors!
np.all(ff==0)
  True

The reason is clearly stated by the code: with that seed, there is no non-negative combination of the PCA templates (at the randomly-generated redshift, which is why this failure is intermittent) which produce a spectrum which passes the QSO color-cuts. Increasing the maximum number of iterations to a number as high as 500 (the default is 20), which takes considerably longer, still returns an all-zero spectrum. So that's not a viable solution. One option would be that if this error condition is reached, the code could turn off color-cuts and throw up a non-catastrophic warning. And indeed, this works:

ff, ww, mm, oo = qq.make_templates(1, seed=458870270, nocolorcuts=True)

OK, I found the problem. The issue is that QSOs are required to have 17.5<r<22.7-- https://github.com/desihub/desitarget/blob/master/py/desitarget/cuts.py#L1225-L1226

However, the default magnitude prior range in templates.QSO.make_templates is 17<r<22.7, and with this seed the randomly generated magnitude is r=17.15, hence no viable template can be generated because the template normalized by this magnitude.

So the trivial fix is to simply update the prior range, which I'll do as part of #534. Going forward, however, we'll have to remember that any changes to the flux cuts in desitarget.cuts should propagate to the default prior ranges in desisim.templates (upon which the desisim unit tests depend).

For example, we could add top-level variables to desitarget.cuts specifying the magnitude ranges, e.g., RBRIGHT_QSO=17.5, based on the latest targeting cuts, which could then be imported by the various template-makers in desisim.templates. That's a little clunky but it will prevent us from being bitten by this issue in the future (especially if we adjust the flux cuts during and after SV, going into the main survey).

@geordie666 @sbailey

sbailey commented 4 years ago

Thanks @moustakas. Please also check LRGs for a similar effect, where we have similar occasional failures.

I'm ambivalent about options for keeping these in sync. Opinions @geordie666 ?

At minimum in the short term @moustakas please include comments in both locations (cuts and templates) that says something like "if you change this value here, also change it here...).

moustakas commented 4 years ago

Thanks @moustakas. Please also check LRGs for a similar effect, where we have similar occasional failures.

Yes, I updated all the template priors in #536.

At minimum in the short term @moustakas please include comments in both locations (cuts and templates) that says something like "if you change this value here, also change it here...).

The right place to put a "warning" would be desitarget.cuts. I can open a ticket in desitarget with my suggestion so we can at least track this issue, unless @geordie666 has another idea.

moustakas commented 4 years ago

Fixed in #536.

moustakas commented 2 years ago

Hopefully fixed for good in #559.