FEAT: Implementing NPY_DT_PyArray_ArrFuncs_compare
#48
Conversation
|
@MaanasArora - any chance you can give this PR a review? |
MaanasArora
left a comment
MaanasArora
left a comment
There was a problem hiding this comment.
Thanks @SwayamInSync, @ngoldbaum, given we use the older ArrFunc API, this looks good to me at least from the NumPy side, just some nits! But do note there is a new DType sort_compare API in numpy/numpy#30415, if that goes in this should be refactored because that registers the new-style array methods instead. But that shouldn't be too hard so I don't think we need to hold this up!
src/csrc/dtype.c
Outdated
| return 0; | ||
| } | ||
| if (a_is_nan) { | ||
| return 1; |
There was a problem hiding this comment.
Maybe add /* NaN goes to the end */ here too?
src/csrc/dtype.c
Outdated
| long double val_b = *(long double *)b; | ||
|
|
||
| // NaN is considered greater than all other values for sorting | ||
| int a_is_nan = (val_a != val_a); |
There was a problem hiding this comment.
Is this safe under all compilers? Is it better to use isnan? Not very familiar with long doubles in C sorry!
There was a problem hiding this comment.
Yeah its portable (IEEE nan property) but I agree its a bad readability, will be better to do isnan
| {NPY_DT_getitem, &quadprec_getitem}, | ||
| {NPY_DT_default_descr, &quadprec_default_descr}, | ||
| {NPY_DT_get_constant, &quadprec_get_constant}, | ||
| {NPY_DT_PyArray_ArrFuncs_compare, &quadprec_compare}, |
There was a problem hiding this comment.
Note, as in the main comment this will be set for deprecation soon, technically is already an older version of the API.
I see so then this old slot will be removed? or we keep both for legacy support? |
|
The old slot won't be removed anytime soon but the new slot is highly recommended, so using it here as soon as it is ready could help encourage dtype authors IMO, and allows descending support too. The ArrayMethod API already allows registering full sort-loops, this is just a missing piece to allow registering sorts just using the compare function. |
| value_str = mp.nstr(mp.mpf(str(value)), 33) | ||
| expected_str = mp.nstr(mp_value, 33) | ||
| assert value_str == expected_str, f"QuadPrecision({val}) ** {pow} = {value_str}, expected {expected_str}" | ||
| class TestSortingOperations: |
juntyr
left a comment
juntyr
left a comment
There was a problem hiding this comment.
The comparison implementation looks good to me
4 participants
closes #46
Copilot Summary
This pull request adds support for sorting and argsort operations for the custom
QuadPrecDType(quad-precision floating point) in both the SLEEF and long double backends. It introduces a new compare function at the C level, registers it with NumPy's dtype slots, and provides comprehensive tests to ensure correct sorting behavior, including handling of special values like NaN and Inf.Sorting support for
QuadPrecDType:quadprec_comparefunction indtype.cthat implements NumPy's compare function, supporting both SLEEF and long double backends, and ensuring NaN values are sorted to the end.QuadPrecDType_Slotsarray so that NumPy's sort and argsort operations can use it.Testing for sorting and argsort:
TestSortingOperationstest suite intest_quaddtype.pyto verify sorting and argsort for various input types, including integers, floats, negative numbers, empty arrays, and arrays with special values (NaN, Inf).quicksort,mergesort,heapsort,stable) for both backends.