-
-
Notifications
You must be signed in to change notification settings - Fork 26k
doc/comment-nan-sort-behaviour-weighted-percentile #31597
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
Conversation
Adds a developer-facing comment to clarify that _weighted_percentile assumes array backends sort NaNs to the end, consistent with NumPy’s behaviour. According to the Array API specification, the sort order of NaNs is implementation-defined and not guaranteed. This clarification helps future maintainers preserve compatibility when integrating new array backends.
❌ Linting issuesThis PR is introducing linting issues. Here's a summary of the issues. Note that you can avoid having linting issues by enabling You can see the details of the linting issues under the
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @AHB30 and thanks for your PR.
The comment is correct and it was discussed like that on the PRs that had implemented array-api on _weighted_percentile
(#29431). However, I actually think that the previous comment # NaN values get sorted to end (largest value)
is enough as a hint to developers who encounter test failures after adding support for another array library. And hence I would not add this long comment.
What do you think, @EmilyXinyi and @lucyleeow?
Yeah I think the open issue is adequate and I wouldn't add a long comment in the code. I daresay the issue would be easier to find than the comment when we decide to add new array support... |
I agree with @lucyleeow and will therefore close this PR. |
Thank you all for the feedback and thoughtful discussion, @StefanieSenger, @lucyleeow, and @EmilyXinyi. I completely understand and agree with your reasoning regarding the balance between clarity and code conciseness. The original comment does already serve as a useful cue, and I appreciate your consideration. Looking forward to contributing to other issues in the future. Thanks again for the opportunity and for maintaining such a high-quality codebase. |
Adds a developer-facing comment to clarify that _weighted_percentile assumes array backends sort NaNs to the end, consistent with NumPy’s behaviour. According to the Array API specification, the sort order of NaNs is implementation-defined and not guaranteed. This clarification helps future maintainers preserve compatibility when integrating new array backends.
Reference Issues/PRs