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

ImmutableRangeMap does not support connected ranges as keys #37

Open
gabbard opened this issue Feb 1, 2019 · 9 comments
Open

ImmutableRangeMap does not support connected ranges as keys #37

gabbard opened this issue Feb 1, 2019 · 9 comments
Assignees
Labels
good first issue Good for newcomers

Comments

@gabbard
Copy link

gabbard commented Feb 1, 2019

For example, [0, 1] and (1, 2] cannot both be keys. This is because we currently use a RangeSet in our implementation, which will coalesce these keys on construction.

For our NLP work this is rarely a problem because we don't use open intervals.

@qpwo
Copy link
Contributor

qpwo commented May 21, 2019

Upon ImmutableRangeMap build, can we tranform half-open or all-open intervals into closed intervals, and keep using RangeSet? Or do we need to replace RangeSet with something new?

@gabbard
Copy link
Author

gabbard commented May 21, 2019

@qpwo : Can you elaborate on what you mean by " transform half-open or all-open intervals into closed intervals", perhaps using the example in the issue description?

@qpwo
Copy link
Contributor

qpwo commented May 21, 2019

Yes, {[0, 1], (1, 2]} would become {[0, 1], [2,2]}. Or {[3,5], (5, 9), [10, 15)} would become {[3,5], [6,8], [10,14]}. (Since we're only dealing with integers right?)

I think the appropriate place to do the change would be at ImmutableRangeMap.Builder.build()

@gabbard
Copy link
Author

gabbard commented May 21, 2019

@qpwo : That would be a fine solution if we were only dealing with integers, but Ranges actually work over arbitrary totally-ordered types.

A really hack-y implementation would be to deal with any connections between a half-closed and a closed interval by making the closed interval also half-closed and storing a separate map for the "connection points". So in the [0, 1] --> foo, (1, 2] --> bar example, you would get [0, 1) --> foo, (1, 2] --> bar plus a separate (internal) map 1 --> bar. But that's really ugly.

I think the solution here is probably to not rely on RangeSet at all for our implementation, though I imagine we can still borrow a lot from it for the implementation.

@qpwo
Copy link
Contributor

qpwo commented May 23, 2019

RangeSet's behavior of merging connected ranges is desired, right?

@gabbard
Copy link
Author

gabbard commented May 23, 2019

Yes

@gabbard
Copy link
Author

gabbard commented May 23, 2019

However, if it were easy to make RangeSet's behavior work either way, you could make it a configurable flag on RangeSet (with merging the

@gabbard gabbard closed this as completed May 23, 2019
@berquist
Copy link
Member

Can someone comment on why this was closed?

@gabbard
Copy link
Author

gabbard commented Jul 30, 2019

@berquist @qpwo : Almost certainly accidental.

@gabbard gabbard reopened this Jul 30, 2019
@qpwo qpwo added the good first issue Good for newcomers label Aug 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants