-
Notifications
You must be signed in to change notification settings - Fork 141
[FEAT]add is_stable #1383
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
base: main
Are you sure you want to change the base?
[FEAT]add is_stable #1383
Conversation
Summary of ChangesHello @deadlywing, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the sorting capabilities by introducing an Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request introduces a is_stable flag to the sorting functions, enabling stable sort capabilities. The changes primarily involve propagating this flag through the function call stack and adding new tests to verify the stable sort behavior. My review includes three points: one critical issue in a test file that would break the build, one high-severity issue regarding a missing check that could cause a runtime error, and one medium-severity comment about an outdated code comment.
| #include <algorithm> | ||
| #include <limits> | ||
| #include <random> | ||
| #include <memory> |
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.
| } | ||
|
|
||
| if (is_stable) { | ||
| return internal::radix_sort(ctx, inputs, direction, num_keys, valid_bits); |
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.
When is_stable is true, internal::radix_sort is called directly. However, radix_sort might not be supported by all protocols, which could lead to a runtime failure if a protocol without the necessary kernels is used. It's important to add a check to ensure that radix sort is supported before calling it, similar to the checks performed later in this function for the default sort method.
SPU_ENFORCE(internal::_check_method_require(ctx, RuntimeConfig::SORT_RADIX),
"Stable sort is requested, but radix sort is not supported by "
"the current protocol.");
return internal::radix_sort(ctx, inputs, direction, num_keys, valid_bits);| // currently, general sort1d only supports odd-even sorting network which is | ||
| // an unstable sort method. |
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.
This comment is now misleading. The sort1d function does support stable sorting when the keys are public by using std::stable_sort. Since this fallback_sort1d function is called from simple_sort1d with public keys, stable sorting is a valid path. Please update the comment to accurately reflect the current implementation.
| // currently, general sort1d only supports odd-even sorting network which is | |
| // an unstable sort method. | |
| // For public inputs, `sort1d` can perform a stable sort. For secret inputs, | |
| // it falls back to an unstable odd-even sorting network. |
Pull Request
What problem does this PR solve?
Issue Number: Fixed #
Possible side effects?
Performance:
Backward compatibility: