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

Fix argsort inside numba.jit using kind='mergesort' #721

Merged
merged 2 commits into from May 4, 2023
Merged

Conversation

dachengx
Copy link
Collaborator

@dachengx dachengx commented May 4, 2023

What is the problem / what does the code in this PR do
As the title described.

This PR is trying to prevent issues like XENONnT/straxen#1175.

Can you briefly describe how it works?

Similar to XENONnT/straxen#1176.

Can you give a minimal working example (or illustrate with a figure)?

Please include the following if applicable:

  • Update the docstring(s)
  • Update the documentation
  • Tests to check the (new) code is working as desired.
  • Does it solve one of the open issues on github?

Please make sure that all automated tests have passed before asking for a review (you can save the PR as a draft otherwise).

@dachengx dachengx requested a review from jmosbacher May 4, 2023 12:40
@@ -306,7 +306,7 @@ def define_run(self: strax.Context,
if isinstance(data, (pd.DataFrame, np.ndarray)):
if isinstance(data, np.ndarray):
data = pd.DataFrame.from_records(data)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just to remove an unused line.

Copy link
Contributor

@jmosbacher jmosbacher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch.
Just wondering if maybe it would be more future-proof if you had the _sort_by_time_and_channel function accept sort_kind as an argument so users could choose other options if they know what they are doing.

@coveralls
Copy link

coveralls commented May 4, 2023

Coverage Status

Coverage: 92.215%. Remained the same when pulling 94101bd on fix_argsort into ce42749 on master.

@dachengx
Copy link
Collaborator Author

dachengx commented May 4, 2023

Nice catch. Just wondering if maybe it would be more future-proof if you had the _sort_by_time_and_channel function accept sort_kind as an argument so users could choose other options if they know what they are doing.

Nice suggestion. Updated.

@jmosbacher
Copy link
Contributor

Maybe only slightly related to this PR, but would be nice at some point to introduce tests that check that all jit compiled functions produce the same results as their pure python implementations

@dachengx dachengx merged commit 73402a3 into master May 4, 2023
10 checks passed
@dachengx dachengx deleted the fix_argsort branch May 4, 2023 15:28
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.

None yet

3 participants