Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

taldcroft
Copy link
Member

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.

@taldcroft taldcroft mentioned this pull request Feb 24, 2019
@jeanconn
Copy link
Contributor

jeanconn commented Feb 25, 2019

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
Copy link
Member Author

Good catch, looking at this.

@jeanconn
Copy link
Contributor

jeanconn commented Feb 25, 2019

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

@jeanconn
Copy link
Contributor

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
Copy link
Member Author

Yup, I cannot reproduce. Phew!
image

@jeanconn
Copy link
Contributor

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

@jeanconn
Copy link
Contributor

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

@taldcroft
Copy link
Member Author

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

@taldcroft taldcroft closed this Feb 25, 2019
@taldcroft taldcroft deleted the dark-ndarray branch February 25, 2019 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants