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

Move apply selection from context to utils #425

Merged

Conversation

JoranAngevaare
Copy link
Member

What is the problem / what does the code in this PR do
context.apply_selection is a very useful function that could be exploited elsewhere too like in (straxens online monitor)[https://github.com/XENONnT/straxen/blob/master/straxen/plugins/online_monitor.py#L324]. We can easily take it out of the context and put it in the strax.utils such that we can more easily use this logic.

I have for now made the function still callable in the context but with a deprecation warning.

@JoranAngevaare JoranAngevaare added the enhancement New feature or request label Apr 11, 2021
@JoranAngevaare
Copy link
Member Author

In c258a0a, I shamelessly copied some nice work from @WenzDaniel in:
https://github.com/XENONnT/straxen/blob/master/tests/test_nveto_events.py

@JoranAngevaare JoranAngevaare added the Testing Works on testing code label Apr 11, 2021
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.

Looks fine, thanks a lot.Just a minor comment.

max_time = draw(hst.integers(*max_time))

data = np.zeros(data_length, dtype=strax.time_fields + [('data', np.float)])
data['time'] = np.random.randint(0, max_time + 1, data_length)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could also draw an array of times https://hypothesis.readthedocs.io/en/latest/numpy.html#numpy then it would be tracked by hypothesis too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks Daniel, I think the input array is now already tracked when failing the tests. Will look at such feature next time I write a similar test.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay as you like. I do not know if hypothesis will track this if an exmapel fails.

Copy link
Member Author

Choose a reason for hiding this comment

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

It does, it's very easy to test right:

================================================= test session starts ==================================================platform linux -- Python 3.8.3, pytest-6.2.0, py-1.9.0, pluggy-0.13.1
rootdir: /mnt/d/Google_Drive/PhD-master/ubuntu-storage/ubuntu-windows/software/strax_official
plugins: hypothesis-5.43.3, cov-2.11.1
collected 4 items

tests/test_utils.py .F..                                                                                         [100%]

======================================================= FAILURES =======================================================_________________________________________________ test_time_selection __________________________________________________
>   ???

tests/test_utils.py:61:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

d = array([],
      dtype=[(('Start time since unix epoch [ns]', 'time'), '<i8'), (('Exclusive end time since unix epoch [ns]', 'endtime'), '<i8'), ('data', '<f8')])
second_time = 0, second_dt = 1

    @settings(deadline=None)
    @given(get_dummy_data(),
           hst.integers(0, 20),
           hst.integers(1, 10),
           )
    def test_time_selection(d, second_time, second_dt):
        """
        Test that both 'touching' and 'fully_contained' give the same
            results as 'strax.fully_contained_in' and
            'strax.touching_windows' respectively
        :param d: test-data from get_dummy_data
        :param second_dt: the ofset w.r.t. the first
        :return: None
        """
>       assert False, d
E       AssertionError: array([],
E               dtype=[(('Start time since unix epoch [ns]', 'time'), '<i8'), (('Exclusive end time since unix epoch [ns]', 'endtime'), '<i8'), ('data', '<f8')])
E       assert False

tests/test_utils.py:74: AssertionError
------------------------------------------------------ Hypothesis ------------------------------------------------------Falsifying example: test_time_selection(
    d=array([],
          dtype=[(('Start time since unix epoch [ns]', 'time'), '<i8'), (('Exclusive end time since unix epoch [ns]', 'endtime'), '<i8'), ('data', '<f8')]),
    second_time=0,
    second_dt=1,
)
=============================================== short test summary info ================================================FAILED tests/test_utils.py::test_time_selection - AssertionError: array([],

Copy link
Collaborator

Choose a reason for hiding this comment

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

No I meant internally in its example cache, to reproduce the exact same condition next time you make a PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see, in principle you can set this as an example, which makes it fully reproducible:

    d=array([],
          dtype=[(('Start time since unix epoch [ns]', 'time'), '<i8'), (('Exclusive end time since unix epoch [ns]', 'endtime'), '<i8'), ('data', '<f8')]),
    second_time=0,
    second_dt=1,

Will take it more into account next time writing such a test. Thanks for showing it!
(This specific one is very unlikely to fail, especially in the parts where this reproducibility is an issue but it's very much appreciated that you correctly pointed this out).

@JoranAngevaare JoranAngevaare merged commit dfc6260 into AxFoundation:master Apr 12, 2021
@JoranAngevaare JoranAngevaare deleted the move_apply_selection branch April 12, 2021 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Testing Works on testing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants