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.

Sign up for GitHub

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

Jump to bottom

Add unify keyword and default if to true #64

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add unify keyword and default if to true #64

wants to merge 1 commit into from

Conversation

Suavesito-Olimpiada
Copy link
Contributor

fix #48

@Suavesito-Olimpiada
Copy link
Contributor Author

Still to add tests and discuss the proper algorithm to unify boxes.

@Suavesito-Olimpiada Suavesito-Olimpiada mentioned this pull request May 27, 2021
Open
@lucaferranti
Copy link
Member

Out of curiosity, do you know how much computational overhead this introduces? Just wondering which one would be a better default value.

Here a simple MWE that might be good for benchmarking

julia> f(x) = x^2 - 2x + 1
f (generic function with 1 method)

julia> minval, minimisers = minimise(f, 0..2, tol=1e-6)

this example should return 2990 minimisers, so it might be a good small but not too small example for benchmarking

@lucaferranti
Copy link
Member

Ah, you answered my questions while I was writing that 😄 Amazing! sorry for the noise

@Suavesito-Olimpiada
Copy link
Contributor Author

Don't worry, happy to help. :D

@dpsanders
Copy link
Member

At a first glance this looks great, thanks! Could you please add some tests?

@Suavesito-Olimpiada
Copy link
Contributor Author

Sure, I'll add test. I just want to know what do you think about the "problem" of using union-find that I described in the comment in the original issue.

dpsanders
dpsanders reviewed May 28, 2021
View reviewed changes
src/utils.jl
return disjoint_sets[i]
end

function add_set!(disjoint_sets, weights_sets, i, j)
Copy link
Member

Choose a reason for hiding this comment

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

What is weights_sets for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is an optimizations for union-find to make the union and find operations almost constant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the fastest asymtotically variant known of union-find. The algorithm is described here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dpsanders dpsanders dpsanders left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unify boxes in result by default?
3 participants
@Suavesito-Olimpiada @lucaferranti @dpsanders