Pickle dark_date always and use it if existing for selecting dark map

[taldcroft]

Trying to avoid this:

In [5]: acas = pickle.load(gzip.open('C://Users//unksm//Downloads/11478_proseco
   ...: .pkl.gz', 'rb'))

In [6]: acas[47759].dark_date
Out[6]: '2019:203'

In [7]: acas[47759].dark
Out[7]:
array([[ 118.17330933,   70.96971893,   20.10654068, ...,  258.32196045,
         411.29510498,  344.21810913],
       [   0.        ,   20.52358055,  167.46463013, ...,   19.52788734,
          22.76825142,    0.        ],
       [   0.        ,   12.25281143,   18.72343826, ...,   67.4727478 ,
          32.30231476,    0.        ],
       ...,
       [ 123.90538788,   70.99282074,  128.89112854, ...,   15.33648682,
           7.7166934 ,    0.        ],
       [ 173.1265564 ,   36.64894485,   16.11512184, ...,  133.4210968 ,
          10.49938488,    0.        ],
       [   0.        ,   39.4043541 ,  114.90283203, ...,   62.78889847,
          14.20818233,  115.17153168]], dtype=float32)

In [8]: acas[47759].dark_date
Out[8]: '2019:288' 

[jeanconn]

Regarding a possible unit test, when reviewing the 2019_206 products, I confirmed that setting the supplied date to get_aca_catalog back in time so (so that the most recent dark current before the input date matched the one we happened to have) worked to give me exactly the catalogs that I expected. So I could fiddle with something like that (manipulating input date and verifying catalogs or some pixels in the dark map or some such) for a unit test for this PR if we care. Do we care?

[jeanconn]

This would be nice to have in ska3-matlab proseco, but now that I understand the issue I think we can work around (not seeing a huge push for this week but at the same time ...)

[taldcroft]

Testing

In actually testing this I realized the implementation was not as useful as we'd like. Here is the new behavior and functional test:

#####
# First, remove the 2006329 directory from the mica dark cal archive.
# This is the "correct" dark cal for this observation on 2007:002
#####

In [7]: from proseco import get_aca_catalog

In [8]: aca = get_aca_catalog(8008)

In [9]: aca.dark_date  # Got the dark cal before 2006:329
Out[9]: '2006:220'

In [10]: pickle.dump(aca, open('aca.pkl', 'wb'))

In [11]:                                                                                                                                
Do you really want to exit ([y]/n)? 

#####
# NOW put the 2006329 dark cal back into mica
#####

(ska3) neptune$ ipython
Python 3.6.2 |Continuum Analytics, Inc.| (default, Jul 20 2017, 13:14:59) 
Type 'copyright', 'credits' or 'license' for more information
IPython 6.1.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import pickle

In [2]: aca = pickle.load(open('aca.pkl', 'rb'))
aca
In [3]: aca.dark_date
Out[3]: '2006:220'

# When we actually access aca.dark then a warning pops up.
# It is still slightly confusing, but no chance of not pausing to
# reflect what happened.
In [4]: aca.dark[1,1]
/Users/aldcroft/git/proseco/proseco/core.py:577: UserWarning: Unexpected dark_date: dark_id nearest dark_date=2006:220 is 2006220 but dark_id before obs date=2007:002:04:31:43.965 is 2006329
  warnings.warn(f'Unexpected dark_date: '
Out[4]: 10.969547

# Pickle file said to use 2006220, so do that!
In [5]: aca.dark_date
Out[5]: '2006:220'

[taldcroft]

I think this is ready and we should include it. Most important for review is that it retains the original behavior for the case of generating a fresh catalog.

[jeanconn]

For me, this fails proseco.tests.test_catalog.test_dark_property()

[jeanconn]

With regard to testing, I was trying to just move around self.date, pickle, and re-load. The only confusion there I have is that we do have a lot of dates (aca.acqs.date, aca.guides.date, aca.fids.date, aca.date). I don't recall if these only exist as an artifact of using this class for everything.

[taldcroft]

Well that dates thing is a feature of using a class structure where acq, guide, and fid selection are viewed as separate processes so each one needs to know its date. So in theory you can change them all independently.

[jeanconn]

Yeah, I was trying to remember if we had a real use case for being able to independently set date or if it was just due to feature (aka artifact) of the class . Fine either way, just harder to define a test that fiddles with it if we wanted to do so.

[taldcroft]

I defined a test that fiddles with the date and it seems to work fine. Each class just cares about its date. The new test is (I think) a whittled down version of the worst-case scenario you gave about a dark cal mismatch from prelim. With any luck if/when that happens there will be a visible UserWarning.

[taldcroft]

I'm in that annoying place where I have already spent way too long on a silly little PR that will probably never get exercised again, and I realize that I failed to run unit tests earlier to see that something weird is happening with how the dark attributes get defined.

So, waste some more time digging into that or just punt on the whole thing??

[jeanconn]

Always hard to know. I still considered this not worth the push, but it is likely that we'll be bit by this a few more times (which I think will generally require a prelim schedule, bad luck, and promotion of an acdc dark cal just after the pickle was made?).

[jeanconn]

Excellent. Looks great and works for me.

I note that we'd probably need something in sparkle to handle warnings (put out to a visible log in the html report?) if we wanted to see the warnings this throws in standard usage. But as I understand it, with this PR, the plots with pixels and the reporting and such should all use the dark current used during selection, so we shouldn't need to be that aware of the warning.

[taldcroft]

Sparkles should spit out a warning to the console when generating the HTML. In our current process I think that would generally be visible. But yes, this should realize the goal of always using the same dark cal in sparkles review as was used in product generation.

[taldcroft]

Should this drive a new release? Seems like a relatively high-value thing to get into the 2019_210 release?

[jeanconn]

I did not see any sparkles warnings when I ran on the test products that started this

[jeanconn]

And I expected new release and was calling it 4.7 in the 2019_210 ska3-matlab PR

[taldcroft]

I did not see any sparkles warnings when I ran on the test products that started this

My guess is because that existing pickle does not have dark_date attributes for acqs, guides, fids, and (probably?) aca.dark is never evaluated in sparkles. So the new code is always falling through to the "no existing dark_date defined" branch. So basically for this system to work we need proseco 4.7 in the product generation loop.

[taldcroft]

My guess seems to be wrong, and this led into the rabbit hole of all the warnings that are actually happening when running sparkles. Try env PYTHONWARNINGS=once sparkles <pickle-file>. In all the mass of output there is not the desired UserWarning. For more fun choose the always option.

[taldcroft]

So contrary to my expectation, sparkles does not actually use the dark attribute at all. No dark, no warning. Sigh.

[jeanconn]

Though did you try with report-level set to something?

[taldcroft]

That might do it. I did confirm that "dark" doesn't appear in the sparkles core code, so until sparkles reaches out to proseco that code will never get triggered.

If this is important we just need to have direct code in sparkles, i.e. when reading in the pickle file then either just do aca.dark or something nicer (not relying on a side effect by reading in a dark cal that won't typically be used).