FEAT: Implementing argmax & argmin PyArray slots for quaddtype#50
FEAT: Implementing argmax & argmin PyArray slots for quaddtype#50SwayamInSync merged 2 commits intonumpy:mainfrom
Conversation
SwayamInSync
left a comment
SwayamInSync
left a comment
There was a problem hiding this comment.
Writing some thoughts down...
| Sleef_quad max_val = *(Sleef_quad *)(data + (*max_ind) * elsize); | ||
|
|
||
| // Skip NaN values | ||
| if (Sleef_iunordq1(val, val)) { |
There was a problem hiding this comment.
It maybe not preferable to use these routines directly in code since we already have their abstractions inside ops.hpp, using them wherever requires might be better as something goes off then just fix that abstraction.
But those abstractions uses C++ features like templates so we might have to rename this file or any other as .cpp
There was a problem hiding this comment.
Pull request overview
This PR implements argmax and argmin PyArray slot functions for the QuadPrecDType custom NumPy dtype, enabling np.argmax() and np.argmin() operations on quad-precision arrays.
Changes:
- Added C implementations of
quadprec_argmaxandquadprec_argminfunctions that handle both SLEEF and longdouble backends - Implemented proper NaN handling (NaN values are ignored unless all values are NaN)
- Added comprehensive test cases covering basic operations, special values (infinity, NaN), and multi-dimensional arrays with axis parameter
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/csrc/dtype.c | Implements argmax and argmin slot functions with support for both backends, proper NaN handling, and registration in PyType_Slot array |
| tests/test_quaddtype.py | Adds test_argmax_argmin function with parameterized tests for both backends covering basic arrays, infinity, NaN, and multi-dimensional arrays |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| npy_intp start = 0; | ||
| Sleef_quad max_val; | ||
| for (start = 0; start < n; start++) { | ||
| max_val = *(Sleef_quad *)(data + start * elsize); |
There was a problem hiding this comment.
That feels wrong (probably does the right thing but ...). What I meant was, after the all-NaN check, load the maxval from the start index before you enter into the loop that finds the total maximum
There was a problem hiding this comment.
Though then again, we're already loading the values ... so this does save one read ... so maybe it's fine or even better?
There was a problem hiding this comment.
That'll be actually worse
- Uses 1 extra temporary variable
valin the NaN-finding loop (it is must to cast the data to quad pointer) - After NaN loop, needs another read:
max_val = *(Sleef_quad *)(data + start * elsize) - Extra memory access (even if cached, it's still an instruction)
2 participants
As per the title, closes #47