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

Fix DisjointSets constructor error #568

Merged
merged 5 commits into from
Jan 24, 2020
Merged

Fix DisjointSets constructor error #568

merged 5 commits into from
Jan 24, 2020

Conversation

c-p-murphy
Copy link
Contributor

Address error introduced by #567 in situations like the following:

DisjointSets(x % 2 == 0 ? x+1//x : x*im for x = 1:10)

@codecov
Copy link

codecov bot commented Jan 19, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@2680058). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #568   +/-   ##
=========================================
  Coverage          ?   87.95%           
=========================================
  Files             ?       32           
  Lines             ?     2109           
  Branches          ?        0           
=========================================
  Hits              ?     1855           
  Misses            ?      254           
  Partials          ?        0
Impacted Files Coverage Δ
src/disjoint_set.jl 94.33% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2680058...7809b81. Read the comment docs.

@c-p-murphy
Copy link
Contributor Author

Just realized this actually still needs work, so don't worry about reviewing or merging it yet.

@oxinabox
Copy link
Member

Definately add that test case . I should have checked coverage before merging

@c-p-murphy
Copy link
Contributor Author

c-p-murphy commented Jan 20, 2020

Everything seems to be working as expected now, but I'm not entirely confident that this is the best approach to solving the problem. Both this and the Set constructor rely on these lines in arrays.jl which seems a bit counterintuitive. If it doesn't seem like a problem to you though, I'm happy to have this merged and then see about opening a new PR down the road if a more elegant solution becomes apparent.

Either way, thanks for taking the time to review this.

Copy link
Member

@oxinabox oxinabox left a comment

Choose a reason for hiding this comment

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

Looks good to me.
Can you bump the patch version so I can tag a release?

@oxinabox oxinabox merged commit 0a49579 into JuliaCollections:master Jan 24, 2020
@c-p-murphy c-p-murphy deleted the fix-disjointsets-error branch January 24, 2020 18:28
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.

2 participants