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

Interval <: IntervalSets.Domain? #13

Open
mcabbott opened this issue May 9, 2020 · 6 comments
Open

Interval <: IntervalSets.Domain? #13

mcabbott opened this issue May 9, 2020 · 6 comments

Comments

@mcabbott
Copy link

mcabbott commented May 9, 2020

Even if this package's interval isn't the same as that in IntervalSets.jl, could it at least be an appropriate subtype?

Ref mcabbott/AxisKeys.jl#13, where that package could use this information to route AcceleratedArrays.Interval back to this package's findall(in(Interval), ...).

@kcajf
Copy link
Contributor

kcajf commented May 9, 2020

Seeing this comment: https://github.com/andyferris/AcceleratedArrays.jl/blob/master/src/Interval.jl#L1 makes me think that the reason for not using IntervalSets is more historical than anything. It might be quite easy to switch over now IntervalSets has matured. @andyferris was there a particular other reason / incompatibility?

@kcajf
Copy link
Contributor

kcajf commented May 9, 2020

I take that back - AA.Interval can have distinct start and end types, which wouldn't be compatible with IntervalSets.Interval.

@andyferris
Copy link
Owner

andyferris commented May 9, 2020

Yes - I'd love to sort this out!

The related issue is JuliaMath/IntervalSets.jl#40

The problem relates to checking containment in the interval via isless or <. You can't sort a collection by < (and < is surprisingly unrelated to isless) and you can't use binary search to efficiently locate all the elements that are in an IntervalSets interval.

(Note: the different endpoints types was added to allow openness/closedness of the bound - one could alternatively have 4 types of interval).

@andyferris
Copy link
Owner

In any case - ideas here are welcome.

I've been thinking of just adding a bunch of pragmatic code to handle < for AbstractFloat and any other common type which has < defined differently to isless. But I haven't got around to it. We can also willfully ignore the semantics of IntervalSets and use it anyway, perhaps leading to a few nasty corner cases but with the benefit of community cohesion.

Thoughts?

@mcabbott
Copy link
Author

mcabbott commented May 9, 2020

Could we just fix IntervalSets to use isless etc, and bump its version number? I doubt that anyone was deeply reliant on these differences of behaviour, and cohesion is worth quite a bit. (But perhaps I should read that thread again more slowly.)

@andyferris
Copy link
Owner

I agree - that would be my preference.

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