-
Notifications
You must be signed in to change notification settings - Fork 49
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
feat[next]: Add missing UnitRange comparison functions #1363
Conversation
src/gt4py/next/common.py
Outdated
if value == Infinity.positive() or value == Infinity.negative(): | ||
# raising error was an adhoc decision, feel free to improve | ||
raise ValueError("Cannot check if Infinity is in UnitRange.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if value == Infinity.positive() or value == Infinity.negative(): | |
# raising error was an adhoc decision, feel free to improve | |
raise ValueError("Cannot check if Infinity is in UnitRange.") |
I would suggest to skip this for performance reasons. Let's talk if you want to know why I care about performance here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, let's talk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments about design and style issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with the changes but I have a few more suggestions and comments.
return (self.start > other.start and self.stop <= other.stop) or ( | ||
self.start >= other.start and self.stop < other.stop | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return (self.start > other.start and self.stop <= other.stop) or ( | |
self.start >= other.start and self.stop < other.stop | |
) | |
return self < other or self == other |
The version I proposed appears much easier to comprehend and now that I see that 4 comparisons are needed performance appears to be the same anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with your proposal we have 2 function calls, 6 comparison and an isinstance check (at least), but I agree that it's simpler to understand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. @havogt's proposal is better for performance reasons, @tehrengruber is better for readability. In this case I think @havogt version is actually better, but both are fine for me.
src/gt4py/next/embedded/common.py
Outdated
new_index = (rng.start if idx >= 0 else rng.stop) + idx | ||
if new_index < rng.start or new_index >= rng.stop: | ||
new_index = (rng.bounded_start if idx >= 0 else rng.bounded_stop) + idx | ||
if new_index < rng.bounded_start or new_index >= rng.bounded_stop: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you use bounded_start
, but don't handle the unbound case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
solved with is_finite checks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one change to conform with the upcoming error message guidelines.
src/gt4py/next/common.py
Outdated
return max(0, self.stop - self.start) | ||
if UnitRange.is_finite(self): | ||
return max(0, self.stop - self.start) | ||
raise ValueError("Cannot compute length of open UnitRange.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
raise ValueError("Cannot compute length of open UnitRange.") | |
raise ValueError("Cannot compute length of open 'UnitRange'.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few style comments but overall looks good so I'm approving the PR because the comments are optional.
return (self.start > other.start and self.stop <= other.stop) or ( | ||
self.start >= other.start and self.stop < other.stop | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. @havogt's proposal is better for performance reasons, @tehrengruber is better for readability. In this case I think @havogt version is actually better, but both are fine for me.
src/gt4py/next/common.py
Outdated
@@ -283,18 +341,21 @@ def named_range(v: tuple[Dimension, RangeLike]) -> NamedRange: | |||
return (v[0], unit_range(v[1])) | |||
|
|||
|
|||
_Rng = TypeVar("_Rng", bound=UnitRange) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a question: is this actually correct? Shouldn't it be _Rng = TypeVar("_Rng", UnitRange, FiniteUnitRange)
instead since FiniteUnitRange
doesn't inherit from UnitRange
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe
_Rng = TypeVar(
"_Rng",
UnitRange[int, int],
UnitRange[Infinity, int],
UnitRange[int, Infinity],
UnitRange[Infinity, Infinity],
)
@tehrengruber I will merge this PR. If you have more comments we can discuss them in a separate PR. |
No description provided.