refactor: slicing mixin class

[DaniBodor]

This is the first step of #129. I decided to make separate PRs for each of those so that each can stay small and comprehensible

This PR refactors the current slicing mixin by (in order of commits):

1. creating a folder structure for all mixin modules and using explicit imports (in the modified modules)
2. fixing some minor errors, rewording errors, etc (see commit message for details)
   • I did some stupid stuff re labeling here. This is solved in later commit (too complex to rebase properly)
3. force expected behavior for start_inclusive and end_inclusive in slice_by_index
4. make labeling part of parent method
5. create test files for this
   • the proper test fails because __eq__ and __len__ are not yet correctly implemented)
   • there's a temporary notebook that can be used to visually check that what we think is happening is indeed happening
6. make slice_by_time result in expected behavior

TO DO:

• add docstrings where needed

[psomhorst]

@DaniBodor I added some comments. (I see that they're not all aligned with the proper lines, VSCode's comment feature has some bugs, I see).

When I wrote this code I was assuming you'd always need an start and end point, but None will just work as intended when slicing.

You moved the generation of the label to the abstract method. This value is never returns or saves anywhere. Maybe consider writing a new function for label generation?

Removing start_inclusive and end_inclusive makes sense for SelectByIndex. However, for SelectByTime this would require a different implementation for the TimeIndexer when selecting a single time point (rather than a slice), and makes it hard to split, change, and merge sequences (a use case I would like to consider enabling).

You removed start_inclusive and end_inclusive as arguments, but they're still used in de code when calling the select_by_...() methods.

As for testing: does it make sense to create a class specific for testing that is purpose built to test slicing? That makes it easier to rule out bugs. Using a complicated class like DraegerEITData which is not finished yet makes debugging harder. I did the same for testing Variants (see VariantSubA and VariantSubB).

[DaniBodor]

@DaniBodor I added some comments. (I see that they're not all aligned with the proper lines, VSCode's comment feature has some bugs, I see).

Thanks for the quick review!

When I wrote this code I was assuming you'd always need an start and end point, but None will just work as intended when slicing.

It does, but then the way I now implemented labeling become more complex, so I made it explicit.

You moved the generation of the label to the abstract method. This value is never returns or saves anywhere. Maybe consider writing a new function for label generation?

this should be resolved now

Removing start_inclusive and end_inclusive makes sense for SelectByIndex. However, for SelectByTime this would require a different implementation for the TimeIndexer when selecting a single time point (rather than a slice), and makes it hard to split, change, and merge sequences (a use case I would like to consider enabling).

Agreed. I changed it back so that index slicing does not have the option, but time slicing does. The end behavior used to be modified by -1, but I dont think that is correct and I changed it. Can you look at it and make sure that this is correct now (see also test notebook)

You removed start_inclusive and end_inclusive as arguments, but they're still used in de code when calling the select_by_...() methods.

fixed

As for testing: does it make sense to create a class specific for testing that is purpose built to test slicing? That makes it easier to rule out bugs. Using a complicated class like DraegerEITData which is not finished yet makes debugging harder. I did the same for testing Variants (see VariantSubA and VariantSubB).

For now I added the notebook that I was using myself for checking that things are correct. This will be removed when proper tests are implemented. In think that at least temporarily solves the problem as well, until everything works properly. Not as fancy, but given that this is a temporary problem (or do you mean as a permanent thing?), this quick solution I think should be ok.

[DaniBodor]

By the way, while reviewing, if you do your comments as "start a review"/"add review comment" rather than "add single comment", then I only get notified when your review is done. This way, I don't get pinged for each comment individually :)

[psomhorst]

By the way, while reviewing, if you do your comments as "start a review"/"add review comment" rather than "add single comment", then I only get notified when your review is done.

Yes, sorry. I added the comments from VS Code, but didn't figure it would ping you for every single one.

[psomhorst]

@DaniBodor The current implementation does not coincide with the discussion we had on slicing by time. As I understood it, we agreed that when slicing by time, the start and end of the time axis should be present inside the time range that is selected if start_inclusive/end_inclusive are True.

E.g. if we slice from start = 10.1 with start_inclusive = True, the resulting time axis should always include the number 10.1. So if the time is [9.85, 10, 10.15, 10.30], the slicing should return [10, 10.15, 10.30]. With start_inclusive = False it should return [10.15, 10.30]. If we slice with start = 10.15 the result should always be [10.15, 10.30], irrespective of the value of start_inclusive.

It could be I'm misunderstanding what we agreed on previously when it comes to slicing time.

[psomhorst]

This code seems to largely function as expected. I'm not quite sure all expectations align, though. Maybe we should re-discuss how time slicing should work, and what it means exactly, and write down some test cases and expected results.

[DaniBodor]

@DaniBodor The current implementation does not coincide with the discussion we had on slicing by time. As I understood it, we agreed that when slicing by time, the start and end of the time axis should be present inside the time range that is selected if start_inclusive/end_inclusive are True.

E.g. if we slice from start = 10.1 with start_inclusive = True, the resulting time axis should always include the number 10.1. So if the time is [9.85, 10, 10.15, 10.30], the slicing should return [10, 10.15, 10.30]. With start_inclusive = False it should return [10.15, 10.30]. If we slice with start = 10.15 the result should always be [10.15, 10.30], irrespective of the value of start_inclusive.

It could be I'm misunderstanding what we agreed on previously when it comes to slicing time.

What you say makes sense. I think it was me who misunderstood the usage of start_inclusive and end_inclusive. Should be fixed now.
What do you think should be the defaults for this?

[DaniBodor]

This code seems to largely function as expected. I'm not quite sure all expectations align, though. Maybe we should re-discuss how time slicing should work, and what it means exactly, and write down some test cases and expected results.

I think I was thinking about it in an illogical way previously. Please see if now it makes sense to you and our expectations are aligned. If still no, then we can maybe discuss it on Friday. I anyway wanted to ask you then what the TimeIndexer class does.