Use np.ndarray instead of ACAImage for dark map

taldcroft commented 5 years ago

The ACAImage class is nice and convenient, but sadly even a few lines of pure Python ends up being really slow for array item access. The change in this PR makes the run time about 75-80% of what it is with current master. So for 30 obsids with a current average time of 1.1 sec, that would shave off 6-7 seconds out of 33.0.

jeanconn commented 5 years ago

I was trying to remember if there is anything special in the ACAImage to deal with edge cases, and then just went to try to make a report for a recent catalog with an edge case. Might need a test for this if there isn't one. I haven't checked yet to see if it is failing on the off-the-ccd star, but the same reporting doesn't fail in master.

In [1]: import pickle

In [2]: acas = pickle.load(open('/data/mpcrit1/mplogs/2019/FEB2519/oflsa/output/FEB2519A_proseco.pkl', 'rb'))

In [6]: import proseco

In [7]: kw = acas['48363'].call_args

In [8]: cat = proseco.get_aca_catalog(**kw)

In [9]: cat.acqs.make_report()
/proj/sot/ska/jeanproj/git/proseco/proseco/report_acq.py:361: VisibleDeprecationWarning: boolean index did not match indexed array along dimension 0; dimension is 178 but corresponding boolean dimension is 388
  bad_stars = acqs.bad_stars_mask[ok]
---------------------------------------------------------------------------
IndexError                                Traceback (most recent call last)
<ipython-input-9-8ce44e148bf5> in <module>()
----> 1 cat.acqs.make_report()

/proj/sot/ska/jeanproj/git/proseco/proseco/acq.py in make_report(self, rootdir)
    206         """
    207         from .report_acq import make_report
--> 208         make_report(self, rootdir=rootdir)
    209 
    210     def update_p_acq_column(self, acqs):

/proj/sot/ska/jeanproj/git/proseco/proseco/report_acq.py in make_report(obsid, rootdir)
    328     make_cand_acqs_report(acqs, cand_acqs, events, context, outdir)
    329     make_initial_cat_report(events, context)
--> 330     make_acq_star_details_report(acqs, cand_acqs, events, context, outdir)
    331     make_optimize_catalog_report(events, context)
    332 

/proj/sot/ska/jeanproj/git/proseco/proseco/report_acq.py in make_acq_star_details_report(acqs, cand_acqs, events, context, obsdir)
    203         cca['spoilers_plot'] = basename
    204         if not filename.exists():
--> 205             plot_spoilers(acq, acqs, filename=filename)
    206 
    207         # Make the acq detail plot with spoilers and imposters

/proj/sot/ska/jeanproj/git/proseco/proseco/report_acq.py in plot_spoilers(acq, acqs, filename)
    359           (np.abs(stars['zang'] - acq['zang']) < plot_hw))
    360     stars = stars[ok]
--> 361     bad_stars = acqs.bad_stars_mask[ok]
    362 
    363     fig = plt.figure(figsize=(5, 5))

IndexError: index 268 is out of bounds for axis 1 with size 178

taldcroft commented 5 years ago

Good catch, looking at this.

jeanconn commented 5 years ago

It seems like it (this particular bad_stars_mask error) should be totally unrelated, but my test environments look good and clean.

jeanconn commented 5 years ago

Nah, there's got to be something wrong with my test directory, as now I can get the same failure on a new checkout of master (though the "wrong" thing in that case would be that it worked in my test repo on "master"). Or this could be the same as that intermittent failure I was having before (if it ends up just working fine for you).

taldcroft commented 5 years ago

Yup, I cannot reproduce. Phew!

jeanconn commented 5 years ago

Great! I'll see if I can narrow down whatever is going on for me again.

jeanconn commented 5 years ago

I think this PR can just get closed as superceded by #292, right?

taldcroft commented 5 years ago

Superceded by #292 which includes the same commit. The issue referenced above is fixed in #294.

sot / proseco

Use np.ndarray instead of ACAImage for dark map #291