Skip to content

Conversation

@spxiwh
Copy link
Contributor

@spxiwh spxiwh commented Sep 10, 2025

This continues work in #5148 by Cythonizing the get_coinc_indices function. For now this is still draft, and I put it here to demonstrate how this might work (especially if anyone in the GRB team is interested in learning some Cython, it would be great to have more people with that knowledge).

Particular things that need doing

  • Writing Cython functions for other common use cases
  • Making the one-detector case a no-op if (as I think) it should be
  • Creating a new .pyx file for GRB Cython functions and moving the new functions there (expecting this will not be the only time we Cythonize bits).
  • Testing (I ran Tito's test case 2 and obtained the same number of triggers (within 1)), I've also ran the code on the same test case generating indices with both the old and new methods and set to raise an error if they differ ... It did not fail (after I fixed the bugs that test uncovered).

Standard information about the request

This is a: efficiency update

This change affects: PyGRB

This change changes: Core scientific code, but only optimization.

This change: has appropriate unit tests, follows style guidelines (See e.g. PEP8), has been proposed using the contribution guidelines

Motivation

#5148 has all the motivation. PyGRB can be slow in some cases. This function was written in "nice" python, but not "fast" python.

Contents

For the 2-detector case this is just finding duplicated values in 2 sorted arrays. There's a well known algorithm for that problem, and I implement it here.

Additional notes

image

Profile (for test case 2) with this PR. It's not as great as I would like, but the dominant cost of get_coinc_indexes remains outside of the Cython function (even though it's doing very little!)

  • The author of this pull request confirms they will adhere to the code of conduct

@titodalcanton titodalcanton added the PyGRB PyGRB development label Sep 10, 2025
@github-project-automation github-project-automation bot moved this to In Progress in PyGRB Development Sep 10, 2025
@spxiwh
Copy link
Contributor Author

spxiwh commented Sep 10, 2025

... I also wonder if caching the output array might fix the 10% of time spent in get_coinc_indexes and not in the Cython function??

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PyGRB PyGRB development

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

2 participants