Deprecate IntSet complement and stored zeros

[mbauman]

As discussed in #10065 (comment), this deprecates complement, complement!, and the ability to explicitly store and test for zero in IntSet, paving the way for a much simpler and more straightforward IndexSet in 0.5. This doesn't do the rename. I figure that can happen when we start changing the internals.

Also fixes a bug in pop!.

(Temporarily without the stored zeros deprecation to try and determine if that was the trigger for the intermittent CI failures)

[yuyichao]

Hmm, a new LLVM assertion from Win 32bit???

[yuyichao]

And a silent worker exit on Travis which is NOT a OOM error?

[jiahao]

I love the name of this branch.

[JeffBezanson]

💯

[mbauman]

I ran this PR through about a dozen CI runs this weekend, with and without the deprecation of storing zero (since I thought that might be what had triggered the assertion failure). All passed. I think this is ready to go once CI gives the green check for the rebase.

[yuyichao]

=). I also checked a few times and didn't see it either. I think we probably don't need to worry about it right now.

[ScottPJones]

👍 (but I really want to see it with the rename, because IntSet is misleading now for what it does)

[Keno]

Hmm, I just came across a use case where the complement feature would have been quite nice. Maybe we should resurrect IntSet in DataStructures?

[StefanKarpinski]

Or compose it with the Complement branch I once had for indices.

[mbauman]

My grand plan has been to move my PR over at #10065 to a ComplementableIndexSets package of some sort, but DataStructures seems like a great home (and much better name). Perhaps that should happen sooner rather than later. My use-case for complements will be for indexing all-but-some indices (#1032). Perhaps it can pun on !(::IndexSet) a little more freely outside of Base, too.

I had forgotten about the complement branch (#4892). That could be a very interesting addition to DataStructures.

[StephenVavasis]

I'm wondering if you can consider rolling back the change to IntSet (that it can no longer hold 0's) and instead focus on the new classes called IndexSet and so on. The manual states that IntSet can hold 'nonnegative' integers, and probably there are many existing codes that count on this behavior.

-- Steve Vavasis

[tkelman]

So I think the idea is that IntSet as used in base Julia has a specific role in the inference code, and for that use case it will help to simplify and unify the implementation to take out the ability to hold 0 and the complement features. The point of deprecation is to give people a bit of warning on this kind of change with a gentle degradation instead of complete breakage.

It sounds like there might be more of a need for this feature than had been thought, so migrating it to DataStructures and making it available there should probably be done sooner rather than later.

[nalimilan]

Why not introduce the changes under a new name like IndexSet? The name IntSet implies that you can hold any Int value there.

[mbauman]

Yeah, I considered that. Moving things out of base has been tough in the past, so that's why I went for modifying it instead. I thought this would be the most graceful transition, but perhaps that's not true.

I'm open to other paths to the end goal here, but it's got to happen soon.

[StephenVavasis]

How about the following plan: leave IntSet in Base and don't deprecate in(0,IntSet()). Create the new class IndexSet also in Base and use this for the internals. Then, if there is time before the 0.4 release, move IntSet to DataStructures, but if there is not, postpone this change until 0.5. To ease the transition, IntSet can temporarily lie in both Base and DataStructures so that users can prepare for the transition early by including the declaration "using DataStructures.IntSet"

By the way, as was pointed out earlier, based on its name, IntSet should really be able to hold any integer. It doesn't seem like much existing code would be broken if IntSet were extended (perhaps in 0.5) to include negative integers as well.

Finally, as I and several others have mentioned in the past, there really should be an abstract class called AbstractSet that sits above IntSet, Set, DataStructures.SortedSet, and the proposed new class IndexSet.

[ScottPJones]

I agree totally with Stephen here

[tkelman]

There isn't time to do any major feature development on this before 0.4. We could simply revert this PR, but then users who hadn't noticed this deprecation on a nightly won't know changes are coming until some time during 0.5-dev. IntSet can temporarily be in both Base and DataStructures already whether or not we revert this, just don't export the DataStructures version from the package while the package is supporting any versions of Julia where the type is present in Base. The version put in DataStructures could be fancier and try to support negative integers without worrying about the development constraints or release schedule of base. The base version of IntSet shouldn't be trying to add new features if the plan is to move it out to a package anyway.

[StefanKarpinski]

I'd favor moving IntSet whole cloth to DataStructures and making IndexSet incompatible with it. We can leave a stub in deprecations indicating that people should use DataStructures if they need IntSet.

[ScottPJones]

Sounds good.

[mbauman]

Please see JuliaCollections/DataStructures.jl#114

[tkelman]

Bump. This has been deprecated since 0.4. Should these have started throwing an error? Deprecations should at least leave a note in deprecated.jl otherwise they'll get lost.

[mbauman]

Ah, awesome. I had totally forgotten about this. I think that we can now move to #10065 without any performance impact since we can now make IntSet immutable. I'll try to resurrect that.