Skip to content
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

[std.algorithm.searching] Add extrema to compute min and max #8727

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ntrel
Copy link
Contributor

@ntrel ntrel commented Mar 25, 2023

Performs < 3n/2 comparisons, unlike the naive < 2n.
Implemented for an input range.
TODO add alias map = a => a parameter, once design is confirmed OK.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @ntrel! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#8727"

@dkorpel
Copy link
Contributor

dkorpel commented Mar 25, 2023

@atilaneves This is a new public function

@atilaneves
Copy link
Contributor

unlike the naive < 2n.

Unlike the naive version done where?

@ntrel
Copy link
Contributor Author

ntrel commented Mar 29, 2023

Unlike the naive version done where?

https://dlang.org/phobos/std_algorithm_iteration.html#fold

// Compute minimum and maximum at the same time
writeln(arr.fold!(min, max)); // tuple(1, 5)

@atilaneves
Copy link
Contributor

Unlike the naive version done where?

https://dlang.org/phobos/std_algorithm_iteration.html#fold

// Compute minimum and maximum at the same time
writeln(arr.fold!(min, max)); // tuple(1, 5)

Do you mean min and max? I don't understand what fold has to do with this.

@pbackus
Copy link
Contributor

pbackus commented Apr 20, 2023

@atilaneves min and max take pairs of elements, not ranges. The versions that take ranges are called minElement and maxElement.

@RazvanN7
Copy link
Collaborator

RazvanN7 commented May 1, 2023

@ntrel @atilaneves how do we move further with this?

@atilaneves
Copy link
Contributor

I still don't know what this is a replacement for.

@dkorpel
Copy link
Contributor

dkorpel commented May 2, 2023

When a user wants to compute the min and max value of a range with Phobos, currently they need to call minElement and maxElement separately, resulting in 2n comparisons. By using the new extrema function, they're only doing 1.5n comparisons. It's somewhat similar to the sincos function being more efficient than separate sin and cos calls.

* r = The range to traverse.
*/
// TODO alias map = a => a
ElementType!Range[2] extrema(Range)(Range r)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The explicit ElementType!Range makes me wonder what happens when the elements are const, immutable, or shared.

@LightBender LightBender added the Salvage This PR needs work, but we want to keep it and it can be salvaged. label Oct 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Salvage This PR needs work, but we want to keep it and it can be salvaged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants