add failing dt test - and solve it with max peak duration #420
Conversation
JoranAngevaare
changed the title
|
We must have been really desperate of tests: |
JoranAngevaare
added
Testing
| # np.iinfo(p['dt'].dtype).max*np.shape(p['data'])[1] = 6553400 ns | ||
| # but numba does not like it |
There was a problem hiding this comment.
I guess it will never. Maybe if you specify a fixed signature to the function, which we usually do not.
| def retrun_1(x): | ||
| """ | ||
| Return 1 for all of the input that can be used as a parameter | ||
| for the splitting in natural breaks | ||
| :param x: any type of array | ||
| :return: ones * len(array) | ||
| """ | ||
| ret = np.ones(len(x)) | ||
| return ret |
There was a problem hiding this comment.
Can we use a lambda function instead? This looks a bit odd.
There was a problem hiding this comment.
lambda is very non pep8 right? What would your main motivation be for making this a lambda?
There was a problem hiding this comment.
Hmm, I to my understanding something like:
function(a, b, c labda x: do_something_simple)
would be the default example for a lambda function. Like used in filter, map etc. It is the only places where I would use them.
There was a problem hiding this comment.
@WenzDaniel , I just stumbled across the pep8 guidelines, and I must say, I would agree with their reasoning:
https://www.python.org/dev/peps/pep-0008/
| peaklets = strax.split_peaks( | ||
| p, r, to_pe, | ||
| algorithm='natural_breaks', | ||
| threshold=retrun_1, | ||
| split_low=True, | ||
| filter_wing_width=70, | ||
| min_area=0, | ||
| do_iterations=2) | ||
| except AssertionError as e: | ||
| if not left_extension + max_duration + right_extension < magic_overflow_time: | ||
| # Ending up here is the ultimate goal of the tests. This | ||
| # means we are hitting github.com/AxFoundation/strax/issues/397 | ||
| print(f'Great, the test worked, we are getting the assertion ' | ||
| f'statement for the int overflow') |
There was a problem hiding this comment.
Would be very strange if we ever trigger this.
There was a problem hiding this comment.
Yeah, this was the design of the tests as I first wrote the test and then implemented the solution.
Co-authored-by: Daniel Wenz <43881800+WenzDaniel@users.noreply.github.com>
|
One more thing I noticed that your changes are also present in: https://github.com/AxFoundation/strax/pull/403/files. We have to be careful with the commit history I think. |
This is true, it's almost exactly the same so should be relatively easy to resolve the merge-conflicts. |
What is the problem / what does the code in this PR do
Add a test that will push us into #397.
Can you briefly describe how it works?
Make a big bunch of records that should be sufficient to break strax with integer overflows. To solve the issue, we should solve this test.
Can you give a minimal working example (or illustrate with a figure)?
This is the example