Remove auto_cast_uint, cast_unsigned, and modify fix_dtype#3982
Conversation
alejoe91
added
core
h-mayorquin
left a comment
h-mayorquin
left a comment
There was a problem hiding this comment.
I strongly agree with this but shouldn't we do some type of deprecation first? so we don't break the code of people who use positional arguments.
|
It was already deprecated. And since next release it's going to be minor I think it's ok to introduce a minor API change. Honwstly I don't think anyone is using |
|
We should have a clear doc on this. |
| if auto_cast_uint: | ||
| cast_unsigned = determine_cast_unsigned(recording, dtype) | ||
| warning_message = ( | ||
| "auto_cast_uint is deprecated and will be removed in 0.103. Use the `unsigned_to_signed` function instead." |
There was a problem hiding this comment.
This seems like a clear enough deprecation to me :)
There was a problem hiding this comment.
I meant in get_traces.
| # if uint --> force int | ||
| if dtype.kind == "u": | ||
| dtype = np.dtype(dtype.str.replace("u", "i")) | ||
| raise ValueError( |
There was a problem hiding this comment.
I would consider making this just a little clearer. I think users would want to know why they aren't supported. So I would recommend adding an additional sentence that says:
"unsigned types are not supported due to **"
I'm just trying to think about what to say? inconsistent behavior in the library? Or to flip it and say that ints and floats perform better.
The issue we are referencing has someone who is saying "I want to use unsigned" so I think we should at least say even in short form why they should not. I agree that maybe some sort of documentation we can point to that explains this or GitHub issue referenced would at least let the users know the real reason why they can't. Especially since this is a change in behavior.
There was a problem hiding this comment.
Sure. We can say that scopy functions don't work well with unsigned ;)
There was a problem hiding this comment.
Missed this but yeah that looks good to me :)
This makes sense. Then by all means, I have been wishing to get rid of this for a while : ) |
4 participants
Fixes #1815