Repository navigation
API: register NEP 35 functions as array_functions - #27490
Conversation
dbe2452 to
a843b6c
Compare
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
Looks quite nice! Since this all is (and was) a little fragile, I think it might be good to add a test that these functions are in fact in the list. Not completely sure what is the best place, but perhaps a new np.testing.tests.test_overrides.py?
|
I wasn't sure how to test this. Are you suggesting a test with a hard-coded list of expected results ? |
No, more a spot check that, e.g., |
|
Gotcha. I did something in between. Let me know if you think it's too much. |
seberg
left a comment
seberg
left a comment
There was a problem hiding this comment.
LGTM, but the test needs to move. Other comments are tweaks, but don't have a quick idea on how and it's also fine as is.
| np.frombuffer, np.fromfile, np.fromiter, np.fromstring, | ||
| np.zeros, | ||
| } | ||
| assert nep35_C_functions.issubset(array_functions) |
There was a problem hiding this comment.
testing is the folder for testing utils, not the folder for tests. This function needs to be in _core/tests/test_overrides.py.
I would expect that the file already has parametrizations for all (or at least most) like= functions.
There was a problem hiding this comment.
thanks ! indeed, the test can easily be rewritten (at least partially) in this module. I just moved it and rewrote it in a redundant style for now, as I see two ways to trim off the redundancy and would like your opinion first:
- only check for the subset that's already parametrized
- expand the existing parametrization to cover the whole (current) set
what option would you prefer ?
| new = getattr(__import__(place, globals(), {}, [obj]), obj) | ||
| if isinstance(doc, str): | ||
| if "${ARRAY_FUNCTION_LIKE}" in doc: | ||
| doc = overrides.get_array_function_like_doc(new, doc) |
There was a problem hiding this comment.
Not sure I love the implicit nature. I think I would prefer a kwarg for this. But I suppose that may be a matter of opinion.
There was a problem hiding this comment.
happy to go with your prefered style. What would you suggest we name the argument ?
There was a problem hiding this comment.
Just go with something that seems sensible to you, I promise not to bike-shed ;).
There was a problem hiding this comment.
just to be clear though, did you mean a kwarg on overrides.get_array_function_like_doc or add_newdoc ? (I assume the former ?)
There was a problem hiding this comment.
I think I meant add_newdoc so that the registration as a like= function isn't based on finding ${ARRAY_FUNCTION_LIKE} in the documentation.
Somehow, I don't love it, but it doesn't matter much. Let's just put it in. Marten also fixed another thing around like= so this will be nice to have.
| "${ARRAY_FUNCTION_LIKE}", | ||
| array_function_like_doc, | ||
| ) | ||
| def get_array_function_like_doc(public_api, docstring_template=None): |
There was a problem hiding this comment.
Would be nice to mention the registration in the name. But if we don't have a quick idea, no worries.
There was a problem hiding this comment.
agreed, I don't love that this function does two things, happy to take suggestions for a better name or pattern, but I don't have one now.
a3d59cc to
2596bb6
Compare
2596bb6 to
de7dd10
Compare
|
@seberg just in case you missed it: I'm waiting on your reply on this thread #27490 (comment) (no rush though) |
Close gh-27451
I aimed for a small footprint. If the additional responsibilities to
add_newdocare not acceptable it should be possible to avoid it, at the cost of more repetitions.I also wasn't able to run the test suite locally yet, so I'm opening as a draft for now.