Repository navigation
fix np.interp type #5231 - #5235
asukaminato0721 wants to merge 3 commits into
Conversation
stroxler
left a comment
There was a problem hiding this comment.
Overall this looks like a good fix, thanks!
Back to you with a suggested improvement in overload structure; also the PR import didn't work so pushing an updated version will probably help me merge it :)
| @overload | ||
| def interp[Shape: _Shape]( | ||
| x: ndarray[Shape] | RegularNestedList[Shape, _ArrayScalar], |
There was a problem hiding this comment.
Can we change this overload to
@overload
def interp[N: IntVar, Rest: _Shape = []](
x: ndarray[[N, *Rest]]
| RegularNestedList[[N, *Rest], _ArrayScalar],
...
) -> ndarray[[N, *Rest]]: ...
so that we rule out matching an empty shape here?
I think if we make that change it might also be able to combine the first and last overloads into just one, because we will have solved the the overlapping-overloads problem.
The current diff uses ordering to try to fix the overlap, but python overload resolution is only partially and ambiguously order-dependent, so it's much cleaner if we can just make the overloads non-overlapping.
|
No auto-import unfortunately :/ I'll try to get this reviewed and merged, thanks! |
|
@stroxler has imported this pull request. If you are a Meta employee, you can view this in D125328983. (Because this pull request was imported automatically, there will not be any future comments.) |
rchen152
left a comment
There was a problem hiding this comment.
Review automatically exported from Phabricator review in Meta.
Summary
Fixes #5231
add missing
__float__andnp.interpTest Plan
add test