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

Support for negative ranges #515

Closed
harry-clarke opened this issue Dec 25, 2022 · 4 comments · Fixed by #518
Closed

Support for negative ranges #515

harry-clarke opened this issue Dec 25, 2022 · 4 comments · Fixed by #518
Assignees
Milestone

Comments

@harry-clarke
Copy link

Hey there,

Am I missing something here?
Negative ranges don't seem to be supported, it seems in part due to string formatting issues (-1-1 confuses the parser)

Is there support for negative ranges, and if not, would others find this a useful addition?

@thiell
Copy link
Collaborator

thiell commented Dec 26, 2022

@harry-clarke indeed it is not supported, we only test use cases with positive indexes. - is parsed as a range separator, not a minus sign. Could you tell us a bit more about your use case where that would be useful?

@harry-clarke
Copy link
Author

Funnily, it's just because I've been making my way through the Advent of Code challenges, and I wanted a RangeSet for intersecting rows and columns with shapes.

In its simplest form, I want to do:

RangeSet(range(-5,7)) - RangeSet(range(1,3))

@harry-clarke
Copy link
Author

At least for now, can we provide a ValueError when passing in ranges with negative values?

I don't mind creating a PR

Ideally, it would be good to remove the dependency on string parsing for interpreting ranges, which should reduce complexity a bit.

@thiell
Copy link
Collaborator

thiell commented Dec 26, 2022

I checked and it is an unexpected change (or "regression") between 1.8.x and 1.9.

In 1.9, we actually changed the way the inner set handles indices: we now rely on strings instead of integers. This allows RangeSet to support complex cases of zero padding, for things like 01-05,001-002 (see
https://clustershell.readthedocs.io/en/latest/tools/nodeset.html?#zero-padding)

With 1.8.4:

>>> RangeSet(range(-5,7))
-5-6
>>> RangeSet(range(1,3))
1-2
>>> RangeSet(range(-5,7)) - RangeSet(range(1,3))
-5-0,3-6

With 1.9:

>>> RangeSet(range(-5,7))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/sthiell/github/thiell/clustershell/lib/ClusterShell/RangeSet.py", line 124, in __init__
    self._parse(pattern)
  File "/home/sthiell/github/thiell/clustershell/lib/ClusterShell/RangeSet.py", line 178, in _parse
    raise RangeSetParseError(subrange, msg)
ClusterShell.RangeSet.RangeSetParseError: cannot convert string to integer : "-5"

For now, you could use 1.8.4, but I will see if we can restore functionality in 1.9.x and officially support negative ranges. I will post updates in this ticket. Thanks for the report!

thiell added a commit to thiell/clustershell that referenced this issue Feb 4, 2023
Support negative ranges in RangeSet, with as little impact as
possible to the normal (positive range) codepath. Zero padding is not
available with negative ranges at this time for this reason.

In case of negative ranges, RangeSet will case the numbers to integer.
Internal sorting then looks like this (eg. -5-6):
['-5', '-4', '-3', '-2', '-1', '0', '1', '2', '3', '4', '5', '6']

Apart from the internal sorting change, little modifications are needed
in the parser and the folding method, so I guess it's worth it.

Usage examples:

    >>> RangeSet(range(-5,7)) - RangeSet(range(1,3))
    -5-0,3-6

    >>> list((RangeSet(range(-5,7)) - RangeSet(range(1,3))).intiter())
    [-5, -4, -3, -2, -1, 0, 3, 4, 5, 6]

    >>> list((RangeSet(range(-5,7)) - RangeSet(range(1,3))).contiguous())
    [-5-0, 3-6]

Hence, the following is now possible:

$ cluset -f foo[-10-10]
foo[-10-10]

$ cluset -f foo[-100--90/2]
foo[-100,-98,-96,-94,-92,-90]

Closes cea-hpc#515.
@thiell thiell added this to the 1.9.1 milestone Feb 8, 2023
@thiell thiell self-assigned this Feb 8, 2023
thiell added a commit that referenced this issue Feb 8, 2023
Support negative ranges in RangeSet, with as little impact as
possible to the normal (positive range) codepath. Zero padding is not
available with negative ranges at this time for this reason.

In case of negative ranges, RangeSet will case the numbers to integer.
Internal sorting then looks like this (eg. -5-6):
['-5', '-4', '-3', '-2', '-1', '0', '1', '2', '3', '4', '5', '6']

Apart from the internal sorting change, little modifications are needed
in the parser and the folding method, so I guess it's worth it.

Usage examples:

    >>> RangeSet(range(-5,7)) - RangeSet(range(1,3))
    -5-0,3-6

    >>> list((RangeSet(range(-5,7)) - RangeSet(range(1,3))).intiter())
    [-5, -4, -3, -2, -1, 0, 3, 4, 5, 6]

    >>> list((RangeSet(range(-5,7)) - RangeSet(range(1,3))).contiguous())
    [-5-0, 3-6]

Hence, the following is now possible:

$ cluset -f foo[-10-10]
foo[-10-10]

$ cluset -f foo[-100--90/2]
foo[-100,-98,-96,-94,-92,-90]

Closes #515.
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

Successfully merging a pull request may close this issue.

2 participants