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 touching window #736

Merged
merged 7 commits into from Jun 22, 2023
Merged

Fix touching window #736

merged 7 commits into from Jun 22, 2023

Conversation

dachengx
Copy link
Collaborator

@dachengx dachengx commented Jun 22, 2023

What is the problem / what does the code in this PR do

In some insane cases, where both endtime of things and container are not sorted, like:

things = np.array([(0, 1, 1, 0),
                   (1, 5, 1, 0),
                   (2, 1, 1, 0)],
                   dtype=strax.interval_dtype)

containers = np.array([(0, 4, 1, 0), (0, 1, 1, 0)],
                       dtype=strax.interval_dtype)

The current algorithm will give wrong results:

[[0, 3], [0, 3]]

The correct answer is:

[[0, 3], [0, 1]]

Can you briefly describe how it works?

The new algorithm only search for interval when the time and endtime of things and containers are sorted. If not, we first sort them and record the argsort, then later recover the order following argsort of input.

When things' endtime are not sorted, we will return the indices of the first and last things which are touching the container. So we return the minimum index in argsort.

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).

@coveralls
Copy link

coveralls commented Jun 22, 2023

Coverage Status

coverage: 91.553% (-0.06%) from 91.609% when pulling 772cea8 on fix_touching_window into 4f19632 on master.

@dachengx dachengx marked this pull request as ready for review June 22, 2023 00:42
@dachengx dachengx requested a review from WenzDaniel June 22, 2023 00:42
@WenzDaniel
Copy link
Collaborator

Hej @dachengx I am a little bit hesitant in merging your PR as the code gets much more complex. I do not know if we really need to cover every single edge case which is actually forbidden based on the provided doc-string. Did not we just add the consistency check for this very same reason?

In addition, I think one should try to avoid using np.append in a numba function as this makes things slow. At least it did in the past.

@dachengx
Copy link
Collaborator Author

Hej @dachengx I am a little bit hesitant in merging your PR as the code gets much more complex. I do not know if we really need to cover every single edge case which is actually forbidden based on the provided doc-string. Did not we just add the consistency check for this very same reason?

In addition, I think one should try to avoid using np.append in a numba function as this makes things slow. At least it did in the past.

The append is removed. Currently, the doc-string allows non-sorted endtime of things and containers. So I think we should cover all of these cases in the function. I do not concern too much about the speed, the new function is ~100% slower than the original one, tested by %timeit. But the original function is very fast.

Copy link
Collaborator

@WenzDaniel WenzDaniel left a comment

Choose a reason for hiding this comment

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

Thanks Dacheng, to me it looks fine. Although I am a little bit concerned performance wise. However, this should now lead to correct results.

strax/processing/general.py Outdated Show resolved Hide resolved
@dachengx dachengx merged commit 2903cf9 into master Jun 22, 2023
10 checks passed
@dachengx dachengx deleted the fix_touching_window branch June 22, 2023 15:56
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