Update datamodel for templates metadata table

[moustakas]

This PR builds on desihub/desitarget#189 by updating the templates code to return a metadata table which conforms to the latest imaging data model (DR4 and beyond). Specifically, DECAM_FLUX[:, 6] and WISE_FLUX[:, 2] have now been split into individual FLUX_G, FLUX_R, FLUX_Z, FLUX_W1, FLUX_W2 columns. See the DR4 data release webpage for more details.

Any code that makes assumptions about the format of the metadata table will need to be updated -- I only fixed code within desisim.

I think I've also addressed the bug identified in #251 but @forero should confirm.

I'm now going to update the associated code called by select_mock_targets.

[moustakas]

@sbailey The failing test is in test_newexp with a "possibly wrong units" error. Before I dig into this and fix it, should I change / update the spectra returned by templates to have the 1e-17 factor in them (and also possibly explicitly use the correct astropy units)? I think this is the only place in either desisim or desispec where this standard is not (currently) being enforced.

[sbailey]

We chatted on phone, but coming up for air on github too: I originally supported "change the datamodel variable names first, reintegrate end-to-end, and then change units later" but from the git traffic it looks like units/tests are getting in the way, so if you need to bit the bullet and convert everything (including the tests) to 1e-17 now, we can update the downstream pieces too.

[moustakas]

@sbailey I decided to move forward with converting the output flux vector from templates to 1e-17 cgs and I think I've fixed most of the unit test failures, at least within desisim. Are there other places (e.g., the integration tests) that I should check?

[sbailey]

If you can get the desisim unit tests working, I'll take care of the integration tests and any knock-on effects to other repos.

[moustakas]

Unless you feel strongly otherwise, in the metadata table I'm going to keep OIIFLUX and HBETAFLUX in plain erg/s/cm2/A, i.e., without the factor of 1e-17.

I'll note here that these line-fluxes do not carry astropy units with them; we should revisit the use of units throughout the simulation workflow at a future date.

[moustakas]

This is ready for review / to be merged, @sbailey.

[sbailey]

synthnano as a 2D array predates this PR, but it is somewhat fragile to have these magic indices tied to the order of the filters specified in the constructor self.decamwise = filters.load_filters.... Especially since part of the motivation for unpacking the decam_flux 2D arrays is to get away from these magic indices, could you do the same here with code something like:

synthnano = dict()
for key in enumerate(maggies.columns):
    synthnano[key] = 1E9 * maggies[key] * magnorm # nanomaggies
...
colormask = self.colorcuts_function(
    gflux=synthnano['decam2014-g'],
    rflux=synthnano['decam2014-r'],
    zflux=synthnano['decam2014-z'],
    w1flux=synthnano['wise2010-W1'],
    w2flux=synthnano['wise2010-W2])

Same comment applies to several other sections with magic indices into synthnano arrays. This caught my eye because the the mapping of indices to filters changed; using a dictionary keyed by filter name would avoid that problem.

[sbailey]

Units bug. This returns an astropy Quantity object with true units of 1e-17 erg/(s cm2 Angstrom) but reported units without the 1e-17.

My preference would be to not use astropy units at all and just return outflux * 1e17 and put in the docstring that the units are 1e-17 erg/(s cm2 Angstrom), though I recognize reasonable arguments for why to provide a units-ful object, especially at certain critical interfaces like this. You also include units in the accompanying metadata table, which is good, which could be an additional argument in favor of using units here.

If you do want to include astropy units to return a Quantity object instead of a vanilla numpy array (your choice), then you shouldn't include the 1e17 factor here.

i.e. don't try to force astropy to return something where str(flux.unit).startswith('1e-17'); astropy will always strip the prefactor for the units and apply it to the numbers to simplify the units. e.g. this would have worked while carrying around unnecessary cancelations:

scaledfluxunits = 1e-17 * u.erg / (u.cm**2 * u.s * u.Angstrom)
return (outflux*1e17) * scaledfluxunits

but even then, flux.unit would be erg / (Angstrom cm2 s) and the values would be O(1e-17)...

Same comment applies to other units-ful return values in other make_templates functions.

[sbailey]

You include unit metadata for the other columns in this table; let's do it for these new columns too.

[moustakas]

This is ready for re-review and merging -- I think I addressed all the issues that were raised. The failing test is a sphinx documentation problem that I'm having trouble debugging, but I'm out of time right now. I'll fix this but if @weaverba137, @sbailey, or anyone else has insight into what I'm doing wrong I'd appreciate it!

[moustakas]

Thank you for the documentation formatting changes, @sbailey. Please merge when ready.

[sbailey]

I fixed sphinx formatting, albeit in a clunky manner. Enough to proceed, but we should revisit how to format our docstrings in a human and sphinx friendly way. Merging now.

[sbailey]

For the record: I merged this before remembering that I was going to check it with downstream integration tests too. Those have probably broken now, but would need to be fixed anyway. I can test some of this with NERSC still out, but other tests will need NERSC to come back (preferably Cori, which is also out tomorrow). But we are probably in a state where master of everything isn't self-consistent.