Vette Frame object

[profxj]

Code to vette the sanctity of a Frame object.
Currently only checks data shape and FLAVOR in meta

Also brings in the concept of parameter files into desispec.
I suspect @sbailey will suggest a modified architecture.

Deprecates the use of 'dark', 'bright', etc. for FLAVOR type.
All are captured by 'science'.

[gdhungana]

Pinging @rstaten for test failure issue on XWSigma test.

[profxj]

Is it possible to make progress on this PR (i.e. the failing
XWSigma test)? I have additional code that builds on this
and I wish this PR to clear before doing the next PR.

[sbailey]

Also brings in the concept of parameter files into desispec.
I suspect @sbailey will suggest a modified architecture.

Not to disappoint, I'll give a minor quibble: changes.rst calls this file data/params/desispec_param.yaml which I like better than the actual implemented name py/desispec/data/params/desi_obj.yml, even if it is hidden by the wrapper function desispec.io.read_obj_param(). I'm not sure what "obj" is supposed to mean in this context and future updates to the parameter file. Maybe just desispec.io.read_params()?

Provided that the tests now pass, please consider that renaming, then go ahead and merge.

For consideration: in similar functions in desimodel, the results of the file are cached so that future calls to the loader don't have to re-read from disk when asking for the same file. If we start fetching these parameters a lot we should do that to limit I/O.

[profxj]

Passing now. @sbailey , be sure to take a look at the
approach to parameters in desispec. Otherwise
may be ready to merge.

[profxj]

Added a test, made params a global (in desispec.io.params),
and renamed the bits and pieces.

[tskisner]

Just a quick comment- if we have thousands of processes that all call "read_params" to load a tiny yaml file, then what happens? Can we be certain that read_params is not called during normal pipeline operations? I guess if we are running from a docker image maybe this matters less, but we should avoid touching the disk except when necessary during pipeline running. Obviously calling Frame.vette() is fine from a QA process running after a given pipeline step.

[profxj]

I think this is a very good point. If we are concerned about I/O then
we should consider coding the params file as a dict. But is loading
a module once per process that much slower than reading a file
from disk once. On the latter point, let me have this cache properly [hold a minute]

[rkehoe]

@profxj et al, If this is in relation to the params governing QAs, then it may be useful to keep in mind that the quicklook QAs take their params as dicts in a configuration file. We are substantially updating that right now, but there are already examples in the quicklook github for this. All of the params have been planned to be configurable. It would be very useful for offline and quicklook to use the same approach, again assuming this thread is referring to the reduction QA params.

[profxj]

It is not referring to QA (today), but we can conflate
it to do so. As such, see what is implemented here
and comment accordingly.

[profxj]

@rkehoe -- Please let me know if you are ok with this PR.
If so, I will ask Stephen to merge.

[rkehoe]

@profxj I do not think the QA params discussion should delay your PR, so please do your merge. We will be asking for a merge on the QuickLook PR very soon, and this will hopefully help a discussion of this topic.

[profxj]

Very good. @sbailey , merge if you are happy.

[sbailey]

I fixed parameter caching; the previous implementation would have ignored the "filename" argument if the parameters had been previously read from a different filename. We may never have hit that situation, but as long as "filename" is an option we should have it fully working. I added tests for this case too.

I also fixed "vette" -> "vet" (past tense is "vetted", but present tense if "vet").

OK to merge when tests pass.

[profxj]

Merging.