type-parameterized IntDisjointSets #599

sbromberger · 2020-03-30T19:02:28Z

This allows the creation of type-parameterized IntDisjointSets, and keeps the default as Int for compatibility. (It also explicitly creates an IntDisjointSets{Int} for DisjointSets, which does not further type-specialize.)

codecov · 2020-03-30T19:21:56Z

Codecov Report

Merging #599 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #599      +/-   ##
==========================================
+ Coverage   95.03%   95.04%   +<.01%     
==========================================
  Files          33       33              
  Lines        2820     2825       +5     
==========================================
+ Hits         2680     2685       +5     
  Misses        140      140

Impacted Files	Coverage Δ
src/DataStructures.jl	`100% <ø> (ø)`	⬆️
src/disjoint_set.jl	`98.66% <100%> (+0.09%)`	⬆️

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 a4246e7...70d32a8. Read the comment docs.

eulerkochy

One slight change is required. rest of it, LGTM

src/disjoint_set.jl

eulerkochy

One small change required, because of Update disjoint_set.jl #600. Make similar changes wherever you notice find_root, both in src and test.
One slight change in tests due to the standard followed in DataStructures.jl is required. I have done it in one place. Please do similar changes.

I think this PR is good to go after that.

src/disjoint_set.jl

test/test_disjoint_set.jl

eulerkochy · 2020-03-31T09:02:45Z

test/test_disjoint_set.jl

+                    @test num_groups(s) == T(10)
+                    @test num_groups(s2) == T(10)
+
+                    for i = 1:10


Suggested change

for i = 1:10

for i in 1:10

in this codebase, the standard way is to use in instead of = for looping over a range. Please correct this too.

OK - but I didn't change this from the original:

DataStructures.jl/test/test_disjoint_set.jl

Line 12 in a4246e7

for i = 1:10

cool .. leave that .. i can take care of it in a separate PR, since it's out of the scope for this PR

Also, since we're changing the signature of find_root, perhaps we should

@deprecate find_root find_root!

?

Sure! I see you have just committed that change.

yup. I think everything's ready to go.

cool. let me take a quick glance.

test/test_disjoint_set.jl

Co-Authored-By: Koustav Chowdhury <kc99.kol@gmail.com>

eulerkochy

Nice work. Thanks for the contribution @sbromberger !! 😃

sbromberger · 2020-03-31T20:39:17Z

Would you please tag a new version after (if) this gets merged? We have a PR that depends on it.

Thank you. ❤️

eulerkochy · 2020-04-01T02:43:42Z

@sbromberger, I have released DataStructures v0.17.11. Hope you can now continue your PR without any further hiccups. 🚀

sbromberger · 2020-04-01T04:38:53Z

Thanks for all your help, @eulerkochy !!

mlubin · 2020-04-03T15:20:53Z

I have a minor complaint. Deprecating find_root in a patch release means, in this case, that JuMP users are now getting deprecation warnings. This is a poor user experience, and bad for the whole ecosystem because if it happens repeatedly we'll have to fix our DataStructures dependency to an exact patch release.

eulerkochy · 2020-04-03T15:27:12Z

@mlubin sorry for that. I should list out the reason for updating find_root to find_root!. The function modifies internally parents due to the use of path compression. So, I thought it would be better to maintain the convention of !. use the patch release v0.17.10, there you wont be facing this issue. Sorry for the bad experience tho'.

sbromberger · 2020-04-03T15:29:45Z

@mlubin we're struggling with this in LightGraphs as well. This is arguably a bugfix - a mutating function did not follow convention - but without the deprecation the bugfix would be a breaking change. With a passthrough nobody would fix it and it would require another release to introduce a deprecation.

Furthermore, I justified it by realizing that below v1.0.0 SemVer rules don't apply. :)

mlubin · 2020-04-03T15:39:52Z

I understand the reasons for the change, but I think it should have a triggered a 0.18 release.

use the patch release v0.17.10

We can update the registry to set JuMP to depend on 0.17.10, but that will inevitably lead to users having version conflicts with other packages that might need a later release (i.e., any packages that have been updated to use find_root!).

I justified it by realizing that below v1.0.0 SemVer rules don't apply. :)

That's true, but the Julia package manager acts as if the SemVer rules for patch releases do apply when it resolves compatible versions. It would be a pretty bad user experience in terms of version conflicts if we had to fix JuMP's dependencies to exact patch releases out of fear that new patch releases might have API changes. A deprecation is an API change.

oxinabox · 2020-04-03T15:55:58Z

I have a minor complaint. Deprecating find_root in a patch release means, in this case, that JuMP users are now getting deprecation warnings. This is a poor user experience, and bad for the whole ecosystem because if it happens repeatedly we'll have to fix our DataStructures dependency to an exact patch release.

This true.

We should think about this longer term and make a policy around this.
It could be that we should something like only add deprecations shortly before a breaking release that removes it?

Hopefully soon we will be up to 1.0 and this can mostly stop.

It would be a pretty bad user experience in terms of version conflicts if we had to fix JuMP's dependencies to exact patch releases out of fear that new patch releases might have API changes. A deprecation is an API change.

A deprecation per semver is a nonbreaking change thus has to be a patch release in 0.x.y.
Removing a deprecations is a breaking change.

One thing I do which basically solves this is to always stay julia --depwarn=no
which is always safe since deprecation removals is a breaking change.

@mlubin would you be able to open an issue to discuss this rather than continuing via comment on this PR?

sbromberger · 2020-04-03T15:57:59Z

Should we revert the deprecation and make it a pass-through (i.e., const find_root = find_root!)?

mlubin · 2020-04-03T16:27:51Z

@oxinabox Do you mean an issue on DataStructures.jl or a broader discussion somewhere?

ccoffrin · 2020-04-03T19:06:33Z

FWIW, I have observed the following impact on some of my work with some JuMP models.

DataStructures v0.17.10 ⚲
model build time: 1.349736 seconds (3.55 M allocations: 415.578 MiB, 6.39% gc time)

DataStructures v0.17.11 ⚲
model build time: 16.691958 seconds (10.46 M allocations: 1.302 GiB, 4.30% gc time)

With v0.17.11 I am seeing the deprecation warning as Miles mentioned. I observed that running julia with --depwarn=no resolves the slow down issue.

DataStructures v0.17.11 ⚲ and julia --depwarn=no
1.380376 seconds (3.55 M allocations: 415.578 MiB, 5.96% gc time)

sbromberger · 2020-04-03T19:19:25Z

Yeah, depwarns slow things down. I think we should revert the depwarn and make a pass through const. I'll propose a PR.

sbromberger · 2020-04-03T19:27:25Z

Ref #603

type-parameterized IntDisjointSets

84308b0

eulerkochy reviewed Mar 30, 2020

View reviewed changes

src/disjoint_set.jl Outdated Show resolved Hide resolved

zero() and one()

3f21d31

eulerkochy approved these changes Mar 30, 2020

View reviewed changes

eulerkochy added the enhancement label Mar 30, 2020

eulerkochy mentioned this pull request Mar 31, 2020

Update disjoint_set.jl #600

Closed

eulerkochy requested changes Mar 31, 2020

View reviewed changes

sbromberger and others added 5 commits March 31, 2020 07:49

Update src/disjoint_set.jl

3c29476

Co-Authored-By: Koustav Chowdhury <kc99.kol@gmail.com>

Update test/test_disjoint_set.jl

88f6f44

Co-Authored-By: Koustav Chowdhury <kc99.kol@gmail.com>

Update src/disjoint_set.jl

7d0b295

Co-Authored-By: Koustav Chowdhury <kc99.kol@gmail.com>

Update test/test_disjoint_set.jl

1ccc415

Co-Authored-By: Koustav Chowdhury <kc99.kol@gmail.com>

depwarn of find_root, changed all instances of find_root

70d32a8

eulerkochy approved these changes Mar 31, 2020

View reviewed changes

sbromberger mentioned this pull request Mar 31, 2020

Dispatch Connectivity, with tests sbromberger/LightGraphs.jl#1381

Merged

eulerkochy merged commit c603ed6 into JuliaCollections:master Apr 1, 2020

sbromberger deleted the sbromberger/IntDisjointSets-parameterization branch April 1, 2020 04:38

mlubin mentioned this pull request Apr 4, 2020

Default to not showing deprecation warnings JuliaLang/julia#35362

Merged

pkj-m mentioned this pull request Apr 6, 2020

Depwarn in matrix_colors JuliaDiff/SparseDiffTools.jl#99

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

type-parameterized IntDisjointSets #599

type-parameterized IntDisjointSets #599

sbromberger commented Mar 30, 2020

codecov bot commented Mar 30, 2020 •

edited

Loading

eulerkochy left a comment

eulerkochy left a comment

eulerkochy Mar 31, 2020

eulerkochy Mar 31, 2020

sbromberger Mar 31, 2020

eulerkochy Mar 31, 2020 •

edited

Loading

sbromberger Mar 31, 2020

eulerkochy Mar 31, 2020

sbromberger Mar 31, 2020

eulerkochy Mar 31, 2020

eulerkochy left a comment

sbromberger commented Mar 31, 2020 •

edited

Loading

eulerkochy commented Apr 1, 2020 •

edited

Loading

sbromberger commented Apr 1, 2020

mlubin commented Apr 3, 2020

eulerkochy commented Apr 3, 2020

sbromberger commented Apr 3, 2020

mlubin commented Apr 3, 2020

oxinabox commented Apr 3, 2020 •

edited

Loading

sbromberger commented Apr 3, 2020

mlubin commented Apr 3, 2020

ccoffrin commented Apr 3, 2020

sbromberger commented Apr 3, 2020

sbromberger commented Apr 3, 2020

type-parameterized IntDisjointSets #599

type-parameterized IntDisjointSets #599

Conversation

sbromberger commented Mar 30, 2020

codecov bot commented Mar 30, 2020 • edited Loading

Codecov Report

eulerkochy left a comment

Choose a reason for hiding this comment

eulerkochy left a comment

Choose a reason for hiding this comment

eulerkochy Mar 31, 2020

Choose a reason for hiding this comment

eulerkochy Mar 31, 2020

Choose a reason for hiding this comment

sbromberger Mar 31, 2020

Choose a reason for hiding this comment

eulerkochy Mar 31, 2020 • edited Loading

Choose a reason for hiding this comment

sbromberger Mar 31, 2020

Choose a reason for hiding this comment

eulerkochy Mar 31, 2020

Choose a reason for hiding this comment

sbromberger Mar 31, 2020

Choose a reason for hiding this comment

eulerkochy Mar 31, 2020

Choose a reason for hiding this comment

eulerkochy left a comment

Choose a reason for hiding this comment

sbromberger commented Mar 31, 2020 • edited Loading

eulerkochy commented Apr 1, 2020 • edited Loading

sbromberger commented Apr 1, 2020

mlubin commented Apr 3, 2020

eulerkochy commented Apr 3, 2020

sbromberger commented Apr 3, 2020

mlubin commented Apr 3, 2020

oxinabox commented Apr 3, 2020 • edited Loading

sbromberger commented Apr 3, 2020

mlubin commented Apr 3, 2020

ccoffrin commented Apr 3, 2020

sbromberger commented Apr 3, 2020

sbromberger commented Apr 3, 2020

codecov bot commented Mar 30, 2020 •

edited

Loading

eulerkochy Mar 31, 2020 •

edited

Loading

sbromberger commented Mar 31, 2020 •

edited

Loading

eulerkochy commented Apr 1, 2020 •

edited

Loading

oxinabox commented Apr 3, 2020 •

edited

Loading