sot / proseco

Probabilistic star evaluation and catalog optimization
https://sot.github.io/proseco
BSD 3-Clause "New" or "Revised" License
1 stars 0 forks source link

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

Closed taldcroft closed 4 years ago

taldcroft commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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.

taldcroft commented 4 years ago

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 commented 4 years ago

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?).

taldcroft commented 4 years ago

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 commented 4 years ago

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

jeanconn commented 4 years ago

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

jeanconn commented 4 years ago

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

taldcroft commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

jeanconn commented 4 years ago

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

taldcroft commented 4 years ago

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).