Skip to content

Conversation

@gsakkis
Copy link
Contributor

@gsakkis gsakkis commented Sep 23, 2022

This PR introduces ConstrainedPartitionsIntRange, an IntRange subclass that will replace the latter in DenseTensorSchema in a follow-up PR.

A ConstrainedPartitionsIntRange is an IntRange whose every subrange (with the possible exception of the first) generated by partition_by_count or partition_by_weight starts from a given range of offsets (start_offsets), in contrast to a regular IntRange whose subranges can start from any offset within the original range.

Example:

In [2]: ir = IntRange(10, 40)

In [3]: [(r.min, r.max) for r in ir.partition_by_count(3)]
Out[3]: [(10, 20), (21, 30), (31, 40)]

In [4]: cpir = ConstrainedPartitionsIntRange(10, 40, start_offsets=range(0, 100, 4))

In [5]: [(r.min, r.max) for r in cpir.partition_by_count(3)]
Out[5]: [(10, 19), (20, 31), (32, 40)]

In this example every subrange (with the possible exception of the first) of cpir starts with a member of start_offsets=range(0, 100, 4).

While at the ranges module, two unrelated changes are included:

@gsakkis gsakkis force-pushed the gsa/InclusiveRange-updates branch from 6aac393 to 1796e35 Compare September 23, 2022 16:24
Copy link
Contributor

@georgeSkoumas georgeSkoumas left a comment

Choose a reason for hiding this comment

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

LGTM!

@abstractmethod
def equal_values(self, other: InclusiveRange[V, W]) -> bool:
"""Check if two ranges consist of the same values."""
return (
Copy link
Contributor

Choose a reason for hiding this comment

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

@gsakkis do we need all the checks here or np.all(self.values == other.values) is enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At minimal we also need len(self) == len(other) because comparing arrays of different size raises DeprecationWarning (and will raise an error in the future). The min/max comparisons are not strictly necessary, they're just an optimization (in case they are false).

@gsakkis gsakkis merged commit 1e7a755 into master Oct 3, 2022
@gsakkis gsakkis deleted the gsa/InclusiveRange-updates branch October 3, 2022 12:57
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 this pull request may close these issues.

3 participants