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

Add is_sorted utility, and use it. #723

Merged
merged 10 commits into from
May 4, 2023
Merged

Add is_sorted utility, and use it. #723

merged 10 commits into from
May 4, 2023

Conversation

matteobachetti
Copy link
Member

@matteobachetti matteobachetti commented Apr 27, 2023

I realized that the code for periodograms was not robust when events were not sorted, so I forced it. I wrote a new is_sorted function, that goes through the array and gives False at the first instance of a non-sorted list (super fast in random arrays, O(n) in quasi-sorted arrays), but it is only fast if numba-compiled. I also used the standard (np.diff(array) >=0).all() implementation if Numba is not installed.
I took the chance to use the new function in a number of places where we used different checks of the same kind, and to fix the test data (that were unsorted themselves).

@codecov
Copy link

codecov bot commented Apr 27, 2023

Codecov Report

Merging #723 (b7d4c94) into main (5f13650) will decrease coverage by 0.06%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##             main     #723      +/-   ##
==========================================
- Coverage   97.22%   97.16%   -0.06%     
==========================================
  Files          42       42              
  Lines        7848     7869      +21     
==========================================
+ Hits         7630     7646      +16     
- Misses        218      223       +5     
Impacted Files Coverage Δ
stingray/utils.py 98.29% <78.94%> (-0.73%) ⬇️
stingray/io.py 98.49% <87.50%> (-0.30%) ⬇️
stingray/gti.py 99.27% <100.00%> (+<0.01%) ⬆️
stingray/lightcurve.py 97.83% <100.00%> (-0.01%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@dhuppenkothen dhuppenkothen left a comment

Choose a reason for hiding this comment

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

One nitpicky question, but otherwise it looks good! 👍

return np.all(np.diff(array) >= 0)
# Test if value is compatible with Numba's type system
try:
_is_sorted_numba(array[:2])
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit confused why you're running _is_sorted_numba twice, once for the first couple of elements, and then for everything? What's happening here?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, is this to check whether the array is extended precision? I feel like there should be a more explicit way to check that rather than just figuring out if Numba fails?

Copy link
Member Author

@matteobachetti matteobachetti May 4, 2023

Choose a reason for hiding this comment

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

It's kind of difficult to test whether an array is extended precision, because it can be called in different ways (np.longdouble, np.float128...), different architecture define these with different precision, and I'm not sure I have full control of all possible ways. Also, I wanted this test to be robust even for other kinds of data (not necessarily extended precision floats) that Numba does not accept and I haven't thought about.

Copy link
Member

Choose a reason for hiding this comment

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

👍

@dhuppenkothen
Copy link
Member

Codecov seems to be complaining?

@matteobachetti
Copy link
Member Author

@dhuppenkothen there is just one line not tested, the one about detector_id which should be trivial ( and needs to change the data files). The numba function is executed (I'm sure of it, because the line that calls that function is executed) but codecov is probably not able to go inside it.

@matteobachetti matteobachetti added this pull request to the merge queue May 4, 2023
Merged via the queue into main with commit d47d50d May 4, 2023
@matteobachetti matteobachetti deleted the add_is_sorted branch May 4, 2023 19:01
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