Add more tests to strax #359

JoranAngevaare · 2020-11-30T08:55:23Z

What is the problem / what does the code in this PR do
Fix a slight mistake in #357 concerning the testing.

Further I've added some other tests while I was waiting on some computations to finish.

…tion/strax into loop_plugin_multioutput

JoranAngevaare · 2020-11-30T16:26:03Z

This is what we like to see

WenzDaniel

Looks mostly fine just some minor comments and suggestions. Took me a while to understand test_loop_plugin, but looks fine too.

WenzDaniel · 2020-12-01T06:55:17Z

tests/test_core.py

@@ -3,7 +3,6 @@
 import tempfile
 import os
 import os.path as osp
-


there probably was an import statement during some point of the live time of this PR

WenzDaniel · 2020-12-01T06:55:47Z

tests/test_loop_plugins.py

-    :param chunks: list op numpy arrays to modify. Here we will drop some of the fields randomly
-    :return: list of chunks
+    Drop some of the data in the chunk
+    :param chunk: list op numpy arrays to modify. Here we will drop some


typo op -> of?

Just to be sure, "chunk" refers here to chunk.data or must I convert a chunk object first into an array?

yeah, it's the chunk.data, also see the type specification in the function definition

let me smooth out the docstring a little

WenzDaniel · 2020-12-01T07:58:08Z

tests/test_peak_properties.py

+
+    # Fill the peaks with random length data
+    for p in peaks:
+        length = np.random.randint(0, data_length)


I am wondering if it would not be nicer to have this already in the hypothesis itself for tracking reasons. You can also create a list of integers.

Can do but as I don't see the how this would affect thinks I consider it unlikely that we ever have to replicate on this specific point. Therefore, I will keep it as it is.

Turns out that this was a good comment though. See #363. Someday will rewrite this test in a similar way!

WenzDaniel · 2020-12-01T08:06:16Z

tests/test_peak_properties.py

+        p['data'] = wf
+
+        for i in range(length, len(wf)):
+            # Set all the out of length data to zero
+            wf[i] = 0.


p['data'][:data_length] = wf does not work?

nope, I also found that strange. Perhaps my syntax was off or its because it's unjitted but yeah

Hmm okay this odd, because I was not sure and so I tried the following in a notebook before I made the comment

peaks = np.zeros(2, dtype=strax.peak_dtype()) for p in peaks: length = int(np.random.uniform(0, 20)) data = np.random.uniform(0, 20, length) p['data'][:length] = data

Ok, so the Perhaps my syntax was off seems quite likely. Will change, thanks Daniel!

tests/test_peak_properties.py

WenzDaniel · 2020-12-01T08:24:33Z

tests/test_peak_properties.py

+    peaks = np.zeros(peak_length, dtype=dtype)
+    dt = 1
+    peaks['time'] = np.arange(peak_length)*dt
+    peaks['dt'][:] = dt


peaks['dt'] = dt should work

WenzDaniel · 2020-12-01T08:26:19Z

tests/test_peak_properties.py

+        p['data'] = wf
+
+        for i in range(length, len(wf)):
+            # Set all the out of length data to zero
+            wf[i] = 0.


See comment above. In general I am wondering if it would not be easier to simply merge this test with the previous one. A lot of duplication in here.

Not impossible but I like the fact that the first test does test something whereon the second test is build.

But this would be also the case if you join them no? But you would just initialize everything once and have less copy-paste code.

OK, will change the setup of the random peaks to a different function, you are right that there is some unnecessary duplication. My reasoning is that I don't mind that much if the tests are sometimes a little rough/slightly duplicate, it's already tedious enough to write them while being quite unrewarding ;).

But this would be also the case if you join them no

So the failure would be a little clearer because you split it over multiple functions with the name of what is being tested.

it's already tedious enough to write them while being quite unrewarding ;).

Yes I know :D I feel with you. But it is our task to gently bully one another for a cleaner code.

OK, will change the setup of the random peaks to a different function,
So the failure would be a little clearer because you split it over multiple functions with the name of what is being tested.

Okay nice, in this way we get both.

WenzDaniel · 2020-12-01T08:34:52Z

tests/test_peak_properties.py

+    if np.sum(peaks['area'] > 0) > 10:
+        mess = ("Highly unlikely that from at least 10 positive area peaks "
+                "none were able to compute the width")
+        assert np.any(peaks['width'] != pre_peaks['width']), mess


The test is very vague, but I can see the difficulty. Maybe we should add a simple unity test like:

data = np.ones(100) *compute widths in 10% steps* *check widths*

It's quite simple right? You just check that the widths were computed at all and not all 0.

WenzDaniel · 2020-12-01T08:41:27Z

tests/test_peak_splitting.py

@@ -39,3 +52,7 @@ def test_LocalMinimumSplitter():
        assert len(my_splits) <= int(len(w) / 2) + 1
        assert min(my_splits[:, 0]) == NO_MORE_SPLITS
        assert my_splits[-1, 0] == NO_MORE_SPLITS
+


L49, L50 would not >= work?

there has to be at least one NO_MORE_SPLITS right. You can also not split on the last entry.

EDIT: Oh wait the lines you are mentioning are different from the ones I'm seeing. But yeah, I tend to agree (although not really part of this PR but originally written by Tim: https://github.com/AxFoundation/strax/pull/267/files).

Yes, sorry there was no other way to comment on this and it just stroke my eye.

WenzDaniel · 2020-12-01T08:52:22Z

tests/test_loop_plugins.py

@@ -112,12 +55,8 @@ def _loop_test_inner(big_data, nchunks, target='added_thing', force_value_error=

    _dtype = big_data.dtype

-    # TODO smarter test. I want to drop some random data from the


L39 typo to -> two

WenzDaniel

Okay looks good.

JoranAngevaare added 6 commits November 30, 2020 09:54

fix loop plugin test

8d687b9

Merge branch 'master' into loop_plugin_multioutput

4f070ae

tweak loop_plugin tests

ef07404

Merge branch 'loop_plugin_multioutput' of https://github.com/AxFounda…

5ca372e

…tion/strax into loop_plugin_multioutput

also test peak splitting

2c64f76

add tests for peak_properties

985ccc5

JoranAngevaare changed the title ~~fix loop plugin test~~ Add more tests to strax Nov 30, 2020

remove silly test

c106d6e

JoranAngevaare requested a review from WenzDaniel November 30, 2020 16:26

WenzDaniel reviewed Dec 1, 2020

View reviewed changes

JoranAngevaare marked this pull request as draft December 1, 2020 10:32

JoranAngevaare added 2 commits December 1, 2020 19:58

address review comments

76276c0

Merge branch 'master' into loop_plugin_multioutput

164a252

JoranAngevaare marked this pull request as ready for review December 2, 2020 09:00

WenzDaniel approved these changes Dec 2, 2020

View reviewed changes

JoranAngevaare merged commit faf309c into master Dec 2, 2020

JoranAngevaare deleted the loop_plugin_multioutput branch December 2, 2020 11:52

JoranAngevaare linked an issue Dec 7, 2020 that may be closed by this pull request

NB breaks test should be reproducible #361

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more tests to strax #359

Add more tests to strax #359

JoranAngevaare commented Nov 30, 2020 •

edited

JoranAngevaare commented Nov 30, 2020

WenzDaniel left a comment

WenzDaniel Dec 1, 2020

JoranAngevaare Dec 1, 2020

WenzDaniel Dec 1, 2020

WenzDaniel Dec 1, 2020

JoranAngevaare Dec 1, 2020

JoranAngevaare Dec 1, 2020

WenzDaniel Dec 1, 2020

JoranAngevaare Dec 1, 2020

JoranAngevaare Dec 7, 2020

WenzDaniel Dec 1, 2020

JoranAngevaare Dec 1, 2020

WenzDaniel Dec 1, 2020 •

edited

JoranAngevaare Dec 1, 2020

WenzDaniel Dec 1, 2020

JoranAngevaare Dec 1, 2020

WenzDaniel Dec 1, 2020

JoranAngevaare Dec 1, 2020

WenzDaniel Dec 1, 2020

JoranAngevaare Dec 1, 2020

WenzDaniel Dec 1, 2020

WenzDaniel Dec 1, 2020

JoranAngevaare Dec 1, 2020

WenzDaniel Dec 1, 2020

JoranAngevaare Dec 1, 2020

WenzDaniel Dec 1, 2020

WenzDaniel Dec 1, 2020

JoranAngevaare Dec 1, 2020

WenzDaniel left a comment

		@@ -112,12 +55,8 @@ def _loop_test_inner(big_data, nchunks, target='added_thing', force_value_error=

		_dtype = big_data.dtype

		# TODO smarter test. I want to drop some random data from the

Add more tests to strax #359

Add more tests to strax #359

Conversation

JoranAngevaare commented Nov 30, 2020 • edited

JoranAngevaare commented Nov 30, 2020

WenzDaniel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WenzDaniel Dec 1, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WenzDaniel left a comment

Choose a reason for hiding this comment

JoranAngevaare commented Nov 30, 2020 •

edited

WenzDaniel Dec 1, 2020 •

edited