Added round option to recording.astype
#2513
Conversation
| @@ -19,6 +21,7 @@ def __init__( | |||
| self, | |||
| recording, | |||
| dtype=None, | |||
| round: bool = False, | |||
There was a problem hiding this comment.
I would put True by default. no ?
There was a problem hiding this comment.
I also think it makes more sense for spike-sorting, but @h-mayorquin made a good point that users might expect the same behaviour as numpy.
I'm fine with both
There was a problem hiding this comment.
Fine with default True. I think that's the preferred default behavior even if it deviates from numpy astype
There was a problem hiding this comment.
Wait, so default ended up being None which means that you round only when integer. Was this discussed outside of the PR or just got through as a non-issue?
There was a problem hiding this comment.
Yes, because rounding is what we normally want in this case. Even though that deviates from numpy behavior, that's better for ephys processing :)
There was a problem hiding this comment.
I think you answering with a question instead of an statement makes it more difficult for me to understand. I am thinking as I write but I feel that something is missing.
Let's make a list to see if I can get you.
- recording dtype is int and you cast to float -> rounding by defaut should not make a difference.
- recording dtype is float and you cast to int -> runding by default was the reason this was added
- recording dtype is int and you cast to int -> rounding by default should not make a difference.
- recording dtype is float and you cast to float -> this would be problematic is someone wanted to cast float 64 to float 32.
Is that last case the problematic one? Am I understanding correctly?
There was a problem hiding this comment.
Yes. In that case the logic would return a float64 but rounded. Do you agree?
There was a problem hiding this comment.
Right. Thanks for explaining.
I propose the following:
I think we can simplify this by making round (preferably called round_float_to_int to be fully descriptive) only True and False.
it should be True bye default as you guys wanted and it can only take two values:
- When
Trueand the input is float, we round before casting to int. If the input is int or the output is float we don't do anything and the function behaes as numpy astype. - When False it defaults to numpy astype behavior for all the cases.
This fullfils the original purpose and it has some advantages over the current approach:
Advantages:
- Easier to document. The behavior is fully described by what I wrote above. We can enchance this by adding examples of why we did this (i.e. inclusive of 0 in np.hisotgram).
- Avoids having the third unecessary branch of
round=Truethat we have now. - Simplifies the type of
round/round_float_to_intby making it boolean only.
There was a problem hiding this comment.
If you change it to round_float_to_int, then you remove the possibility that someone, for some reason, would want to round but keep it as a float.
I don't see what the problem would be when converting from float64 to float32?*
By default there would be no rounding (as there is no need), but the user ca explicitly ask for it if he wants to
There was a problem hiding this comment.
Yeah, I want to reduce the generality of the code to make it easier. Less general code is good. Easier to understand, mantain docstrings (which are lacking in this case) and write tests if needed. Plus, in this case I can't really see a use case for the generalization, that is, it does not buy us anything other than generality for the sake of generality. If the use case comes to be, then we will be in a better position to write a general implementation with concrete use case in mind.
The cost of generality on the other hand? it took me some time to understand what each of the options does and where and why I would want them. Not all cases are documented which illustrates my point that is more work We can save a future developer time by making this more concrete and save ourselves the work to document cases that are unlikely to be used. Plus, it is is way easier to document something that is more concrete. Case in point: try writing a docstring for the more general case of the CAR preprocessor, no one has done it for a reason.
| @@ -19,20 +21,26 @@ def __init__( | |||
| self, | |||
| recording, | |||
| dtype=None, | |||
| round: bool | None = None, | |||
There was a problem hiding this comment.
Can you add a small doc please ?
There was a problem hiding this comment.
The case of False which is recovering numpy default behavior is not documented.
For future developers maybe we can add a comment saying that we do this because np.histogram is inclusive at 0 and we do this to counteract that.
By default
Falseto mimic numpy'sastype