Feature: time-slicing function for pyfar.Signal objects

[h-chmeruk]

Which issue(s) are closed by this pull request?

Closes #751

This is my implementation of the pyfar.dsp.time_crop function, which can be used to crop a signal in the time domain. I have also written some test functions, most of which are similar to the existing test functions for pyfar.dsp.time_window.

[ahms5]

nice, thank you. a few minor comments on the doc and testing

[ahms5]

see previous review

[jf-som]

Thanks for implementing this feature!

Adding to what @ahms5 already mentioned, I just have some coding style hints :)

[h-chmeruk]

I added TimeData as a data type for the parameter signal, so the time_crop function has had some changes, and there are also some significant changes in test_dsp.py . Therefore, I could use a deeper review again. Thanks in advance :)

[ahms5]

Thank you hanna, Looks fine, just one main point about the error handling, see below.

[jf-som]

Thanks for implementing these changes. I found a small corrention and an unusual empty line, but otherwise all looks good to me

[sikersten]

Thank you! Already looks quite good. Some minor comments and one more important on the function's behavior if interval is out of the boundaries.

[mberz]

Thanks for the implementation.
Apart from the interval checking issue missing for samples already mentioned by Simon I have only minor comments. Functionality wise I think this is good to go.

Additionally I found a couple of indenting issue which do not comply with PEP8. You can have a look at the guidelines for PEP8 here:
https://peps.python.org/pep-0008/#indentation

[mberz]

I would suggest to add a sentence on the definition of the crop interval used by the function, i.e. that the function crops to the inside of the interval.

[jf-som]

Thanks for implementing the changes. In terms of the actual code, this looks ready to me.

I have two general design thoughts though:

1. Intuitively, I would expect this function to behave like slicing in numpy or python in general, where the interval [3, 6] gives the indexes 3, 4 and 5 (excluding 6). So
   cropped = pf.Signal(signal.time[:, 3:6], signal.sampling_rate) should be the same as
   cropped = pf.dsp.time_crop(signal, [3, 6]).
   As is it is implemented now, the interval end is inclusive, which is unusual for 0-indexed programming languages. So I would argue to replace indices <= interval[1] by indices < interval[1].
   I'm aware that time_window also has an inclusive range. But since time_crop has a different goal that is closer to array slicing, I would argue that the inlusive start, exclusive end interval is more appropriate here.

I see you added TimeData as a an alternative input to Signal. In my opinion, it's unusual to allow different types of inputs and return types, when the behavior of the function is different depending on the type. In this case, cropping TimeData seems to keep timestamps while Signal changes them to start at 0, as you show in the examples.
Since the return type is always the same as the first argument, it might make more sense to make two instance methods out of this with different comments, i.e.

croppedTimeData = someTimeData.time_crop(interval, unit)
croppedSignal = someSignal.time_crop(interval, unit)

This version would be similar to the filter.impulse_response() functions, which also do slightly different things depending on the type of filter.
Doing so would also help with type hints in vscode etc, since then the IDE could infer the output type from the object it was used on.

Perhaps we can discuss this in the weekly meeting in 7 days.

[h-chmeruk]

Thanks for implementing the changes. In terms of the actual code, this looks ready to me.

I have two general design thoughts though:

1. Intuitively, I would expect this function to behave like slicing in numpy or python in general, where the interval [3, 6] gives the indexes 3, 4 and 5 (excluding 6). So
   `cropped = pf.Signal(signal.time[:, 3:6], signal.sampling_rate)` should be the same as
   `cropped = pf.dsp.time_crop(signal, [3, 6])`.
   As is it is implemented now, the interval end is inclusive, which is unusual for 0-indexed programming languages. So I would argue to replace `indices <= interval[1]` by `indices < interval[1]`.
   I'm aware that `time_window` also has an inclusive range. But since time_crop has a different goal that is closer to array slicing, I would argue that the inlusive start, exclusive end interval is more appropriate here.

I see you added TimeData as a an alternative input to Signal. In my opinion, it's unusual to allow different types of inputs and return types, when the behavior of the function is different depending on the type. In this case, cropping TimeData seems to keep timestamps while Signal changes them to start at 0, as you show in the examples. Since the return type is always the same as the first argument, it might make more sense to make two instance methods out of this with different comments, i.e.

croppedTimeData = someTimeData.time_crop(interval, unit)
croppedSignal = someSignal.time_crop(interval, unit)

This version would be similar to the filter.impulse_response() functions, which also do slightly different things depending on the type of filter. Doing so would also help with type hints in vscode etc, since then the IDE could infer the output type from the object it was used on.

Perhaps we can discuss this in the weekly meeting in 7 days.

Thank you very much for such a comprehensive review! I appreciate your opinion.

Regarding the first point: I don't use pyfar myself, so it's always difficult for me to evaluate the context in which the implemented functions will be used. If the majority agrees that the end of the interval should not be included in the new signal, I will of course apply these changes.

Regarding the second point: I would like to note that the idea of adding TimeData to this function as alternative data type for the parameter signal has been around since one of the very first comments on this PR. Speaking spontaneously, I have no problem with turning this function into an instance method. However, next time I would appreciate knowing in advance if there are any opinions against combining two different data types in one function.
But, as with the interval end, if the majority agrees that the function should be turned into an instance method, I will certainly do so.

And I agree that this is a topic for discussion at the weekly meeting :)

[jf-som]

Your are right in that it's pretty late for structure changes like this, though the thought only came to me now that I saw the different examples in practice... Sorry!

For now I just think it's something worth discussing. If we decide to make it instance methods instead, I don't think it will be too much work. But I'm also open to implement that change myself if it comes to it, I'm not trying to offload extra work onto you ^^

[h-chmeruk]

Your are right in that it's pretty late for structure changes like this, though the thought only came to me now that I saw the different examples in practice... Sorry!

For now I just think it's something worth discussing. If we decide to make it instance methods instead, I don't think it will be too much work. But I'm also open to implement that change myself if it comes to it, I'm not trying to offload extra work onto you ^^

It's fine, don't worry, I also think that implementing this change won't take long. And it's certainly better to mention possible improvements belatedly than not to mention them at all.
So thank you again ^-^

[jf-som]

As discussed in the weekly, I consider this ready for merge. 🙌

As a last minor edit, it would be nice if you could add a short hint to the different signal.times behavior between Signal and TimeData input types before the merge, like we discussed in the weekly.

Thanks for implementation!

[ahms5]

approved, execpt hint about the interval, feel free to ajust my recommendation.
Thank you Hanna.