-
Notifications
You must be signed in to change notification settings - Fork 191
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
Improve handling of coeff
in filter.py
#3243
Comments
That was a huge debate earlier because there are three options (not including yours):
I think we also briefly discussed just putting |
coeff
in filter.py
coeff
in filter.py
The previous discussion was about ids which we allow list and array, then @DradeAW likes to use tuples and he gave some arguments to add tuple. I honestly don't remember where this was discussion took place so I can't link it. Probably slack?
Sequences does not work because a single string is of type sequence but we don't take that. There is some recent debate among the python type developers about this. See this: |
I think that something from numpy typing module could work. |
You're right. I always forget that str is a sequence. Which has led to issues in the library itself.
I think NumPy typing is even more confusing for new people. Their typing for "dtype" is |
Why do you tink that numpy is more confusing?
https://numpy.org/devdocs/reference/typing.html#numpy.typing.ArrayLike What I am missing or how you are thinking about it? |
No I think it is clear for us, but I like to think of the mental burden of brand new people. |
Don't most scientific programmers learn arrays (matlab or numpy) before learning list, tuples and the native data structures of python? I thought that arrays would be more natural to them (i.e. vector like) than an explicit enumeration of the types.
I did not answer to this. |
Again I don't want to misquote you, but I roughly remember you saying you guys did this at neuroconv where you had something like
I don't think people learn tuple at all. I would say list would still be relatively common. I was helping some people with the intro to computing in neuro class here at Harvard and it was a lot of native python moving onto numpy structures later. Part of the problem is that arrays are called vectors or matrices in Matlab so the inherent link between the array object/structure and the mathematical entity is not necessary clear when learning Matlab. Even Numpy originally had matrix stuff which was deprecated because really everything is an array. But I would opine that even scientific programmers may not make the link right away between array and matrix. But again I'm fine being overruled. I probably tend to assume too little knowledge in users. |
No worries. I think the neuroconv example is that where we encapsulated FolderPath like for str|Path and also FilePath for str | Path. In that case it is too few options for this to make sense to me and the cost of having to learn where to import this (as I needed to build functions with that signature) did not seem to be worth the cost. But in this case, there are many things (not just Path | str) and it is kind of obscure what it means so I think it makes sense.
Fair enough, this is evidence against my belief. So to summarize, I agree to encapsulate if there are many types (like with ArrayLike) but we really need a good name. If ArrayLike does not do it for this case and the ids I don't really anything better to offer than being explicit. |
(Apologies both, will comment at some point on the above discussion!) When taking care of these documented changes on |
Just a small thing, leaving as a reminder for myself to open a PR after #3172. At present
coeff
in filter.py takestuple[np.array] | list[np.array] | np.array | None
. This needs reflecting in the docstring, and the numpy array are cast to list in the class constructer but I don't think its necessary, thenp.array
can be passed directly to the scipy functions.In general, if an argument can take
list[x] | tuple[x]
what shall we do? It seems verbose to include both in docstring. Shall we always set totuple
(unless the variable is meant to be mutated in place, which I think is not likely in most cases)?The text was updated successfully, but these errors were encountered: