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

dispatch loop on missing non-integer intersect() method for ranges #7760

Closed
quinnj opened this issue Jul 28, 2014 · 8 comments
Closed

dispatch loop on missing non-integer intersect() method for ranges #7760

quinnj opened this issue Jul 28, 2014 · 8 comments

Comments

@quinnj
Copy link
Member

quinnj commented Jul 28, 2014

Right now intersect() for ranges is only defined for Integer, so that means we have

In  [42]: intersect(1.0:10.0,5.0:15.0)

Out [42]: ERROR: stack overflow
while loading In[42], in expression starting on line 1

Because it recursively calls the intersect(r::Range, s::Range...) definition. The same behavior is obviously the same for date ranges (which is how I discovered this).

I'm sure it's not a walk in the park to get something working generically for ranges, but I'll start playing around with it unless someone else has a better idea.

@ivarne
Copy link
Sponsor Member

ivarne commented Jul 28, 2014

5.0:15.0 is a FloatRange, not a StepRange.
5:15 is a UnitRange.

Because of rounding issues when slicing FloatRangees, I'm not sure intersect can really work. See #7420.

The stack overflow should be fixed though. Is it enough to change it to?

function intersect(r1::Range, r2::Range, r3::Range, r::Range...)
    i = intersect(r1, r2)
    i = intersect(i, r3) # Added this line after @quinnj's comment
    for t in r
        i = intersect(i, t)
    end 
    i   
end

@quinnj quinnj changed the title Missing non-integer intersect() method Missing non-integer intersect() method for ranges Jul 28, 2014
@quinnj
Copy link
Member Author

quinnj commented Jul 28, 2014

Yep, sorry for the over-use of StepRange; that was how I discovered it, but I've edited the description to "all non-integer ranges", so FloatRanges as well as non-Integer StepRanges.

I'm sure FloatRanges would be a pain and a half, as well as a generic StepRange implementation. I think I'll start by trying to do intersect{T<:TimeType}(::StepRange{T},::StepRange{T}) and see how far I get.

@quinnj
Copy link
Member Author

quinnj commented Jul 28, 2014

And no, your definition above doesn't change anything since r::Range... also includes r with length(r) == 0, so it will recursively call itself as well.

@ivarne
Copy link
Sponsor Member

ivarne commented Jul 28, 2014

Sorry, updated the definition to take yet another Range.

@JeffBezanson JeffBezanson changed the title Missing non-integer intersect() method for ranges dispatch loop on missing non-integer intersect() method for ranges Jul 28, 2014
@quinnj
Copy link
Member Author

quinnj commented Jul 29, 2014

@JeffBezanson, could we remove the type restraint on intersect to intersect{T}(::StepRange{T},::StepRange{T}), that would allow date ranges to reuse this implementation (it only took defining gcdx and lcm for Period types)?

@JeffBezanson
Copy link
Sponsor Member

Sure; go ahead and pull request the changes you want for that.

@ivarne
Copy link
Sponsor Member

ivarne commented Jul 29, 2014

The fix for this should probably be limited to OrdinalRange. Seems likely that ranges that isn't OrdinalRange, is not required to have an efficient binary intersect method, and might benefit from the general iterator based version.

@ivarne
Copy link
Sponsor Member

ivarne commented Jul 29, 2014

For intersections of 3 or more objects.

JeffBezanson added a commit that referenced this issue Aug 11, 2014
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

No branches or pull requests

3 participants