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

issubset aces Discrete Math but flunks Topology #28871

Merged
merged 1 commit into from
Aug 26, 2018
Merged

Conversation

timholy
Copy link
Member

@timholy timholy commented Aug 24, 2018

#26198 did some fantastic performance tuning of issubset based on a very thorough analysis of scaling behavior. However, the change implicitly assumed that the "superset" is countable, specifically that length is defined and will return an integer value. Ranges are an example of a container for which this succeeds (this example leverages the fact that numbers are iterable):

julia> r = 1:3
1:3

julia> issubset(2, r)
true

julia> issubset(4, r)
false

So far so good. However, length can't be reasonably defined for many mathematical sets, and as a consequence bad things happen in issubset:

julia> struct OpenInterval{T}
           lower::T
           upper::T
       end

julia> Base.in(x, i::OpenInterval) = i.lower < x < i.upper

julia> issubset(3, OpenInterval(2, 4))
ERROR: MethodError: no method matching length(::OpenInterval{Int64})
Closest candidates are:
  length(::Core.SimpleVector) at essentials.jl:571
  length(::Base.MethodList) at reflection.jl:728
  length(::Core.MethodTable) at reflection.jl:802
  ...
Stacktrace:
 [1] issubset(::Int64, ::OpenInterval{Int64}) at ./abstractset.jl:230
 [2] top-level scope at none:0

This PR addresses the issue in a minimalistic fashion: it checks the IteratorSize trait of the superset, and skips the performance-tuning block if length can't reasonably be expected to succeed.

This isn't an ideal solution, because HasLength() is the default, but at least it gives package authors a means to avoid the bad behavior.

@oscardssmith
Copy link
Member

does this work with testing if open intervals subset other open intervals?

@KristofferC KristofferC mentioned this pull request Aug 24, 2018
@timholy
Copy link
Member Author

timholy commented Aug 25, 2018

does this work with testing if open intervals subset other open intervals?

Assuming an OpenInterval is not iterable, no. But that's fine, because the docstring of issubset is

help?> issubset
search: issubset

  issubset(a, b)
  (a,b)  -> Bool
  (b, a) -> Bool

  Determine whether every element of a is also in b, using in.

so issubset isn't expected to work in that case. But we do need a way to live up to the promise of just testing against in; the performance-tuning shouldn't break the fact that there is a very simple definition that does work.

(EDIT: the docstring doesn't preclude a specialized definition, of course. But there isn't a reasonable fallback definition if the first argument isn't iterable.)


if rlen > lenthresh && !isa(r, AbstractSet)
return issubset(l, Set(r))
if IteratorSize(r) isa Union{HasLength,HasShape}
Copy link
Member

Choose a reason for hiding this comment

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

There is already a function defined for that: Base.haslength(r) ;-)

@timholy
Copy link
Member Author

timholy commented Aug 25, 2018

Updated to use haslength. With two approvals and no objections, I think we can merge this once it gets through CI again (it passes locally).

@KristofferC KristofferC merged commit 24de931 into master Aug 26, 2018
@KristofferC KristofferC deleted the teh/issubset branch August 26, 2018 09:52
KristofferC pushed a commit that referenced this pull request Aug 26, 2018
KristofferC pushed a commit that referenced this pull request Sep 8, 2018
KristofferC pushed a commit that referenced this pull request Sep 8, 2018
@KristofferC KristofferC added bugfix This change fixes an existing bug and removed backport pending 1.0 labels Sep 27, 2018
KristofferC pushed a commit that referenced this pull request Feb 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants