You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This is a very old issue that I discussed with @mtlam years ago but somehow never actually resolved.
Despite the fact that it does basically the same thing, ar.tscrunch() is hundreds of times slower than ar.scrunch('T'). The culprit is this set of nested for loops.
Normally you can work around this by avoiding the former in favor of the latter, but ar.getLevels() (used by ar.getPulsarCalibrator()) calls self.tscrunch() internally here, and as a result is much slower than it needs to be. I ran into this in the quicklook notebook, and found that patching PyPulse to replace self.tscrunch() with self.scrunch('T') in getPulsarCalibrator() improved the runtime of the relevant cell from ~45 s to ~72 ms, and the runtime of the entire notebook from ~1 minute to ~15 s.
I'm not sure what the best way to fix this is. The simplest thing to do would be to apply the patch I described above, but that leaves the current behavior of tscrunch() as a trap for the unwary. Possibly tscrunch() should be removed altogether, but it does have some functionality that's not replicated by scrunch('T') -- namely, the ability to scrunch by an arbitrary factor. The current behavior is somewhat broken, though, in that when the factor doesn't evenly divide the number of subints, the last subint in the resulting scrunched archive will invariably consist of all zeros.
This is great work, going to merge #17 in. Adding a note here since we've discussed a bit in slack that we should rework scrunching in general so that there aren't these two methods. I do like the arbitrary factor functionality but if the reduceat isn't sufficient then we can also implement if statements if the axis is evenly divisible. In general it would be good to move out some of this code into a utility function as there have been requests over the years from people to scrunch an arbitrary numpy array, requiring people to copy the functionality from PyPulse code itself. So I think that's something we should provide in one function and then call that outside. But, for another time. Until then, will keep this issue open.
Activity
mtlam commentedon Jun 11, 2024
This is great work, going to merge #17 in. Adding a note here since we've discussed a bit in slack that we should rework scrunching in general so that there aren't these two methods. I do like the arbitrary factor functionality but if the
reduceatisn't sufficient then we can also implement if statements if the axis is evenly divisible. In general it would be good to move out some of this code into a utility function as there have been requests over the years from people to scrunch an arbitrary numpy array, requiring people to copy the functionality from PyPulse code itself. So I think that's something we should provide in one function and then call that outside. But, for another time. Until then, will keep this issue open.