Skip to content

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.

@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.

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.

@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 93.75000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.12%. Comparing base (24e9808) to head (6c2637f).
⚠️ Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
src/utils.jl 92.59% 2 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #64      +/-   ##
==========================================
+ Coverage   91.83%   92.12%   +0.28%     
==========================================
  Files           5        6       +1     
  Lines          98      127      +29     
==========================================
+ Hits           90      117      +27     
- Misses          8       10       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unify boxes in result by default?

4 participants