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

feat[next]: Add missing UnitRange comparison functions #1363

Merged
merged 37 commits into from
Dec 18, 2023

Conversation

havogt
Copy link
Contributor

@havogt havogt commented Nov 17, 2023

No description provided.

@havogt havogt requested a review from tehrengruber November 20, 2023 07:33
src/gt4py/next/common.py Outdated Show resolved Hide resolved
Comment on lines 147 to 149
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.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, let's talk

src/gt4py/next/common.py Outdated Show resolved Hide resolved
tests/next_tests/unit_tests/test_common.py Outdated Show resolved Hide resolved
Copy link
Contributor

@egparedes egparedes left a 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.

src/gt4py/next/common.py Outdated Show resolved Hide resolved
src/gt4py/next/common.py Outdated Show resolved Hide resolved
src/gt4py/next/common.py Outdated Show resolved Hide resolved
src/gt4py/next/common.py Outdated Show resolved Hide resolved
src/gt4py/next/common.py Outdated Show resolved Hide resolved
src/gt4py/next/common.py Outdated Show resolved Hide resolved
src/gt4py/next/common.py Outdated Show resolved Hide resolved
src/gt4py/next/common.py Outdated Show resolved Hide resolved
tests/next_tests/unit_tests/test_common.py Outdated Show resolved Hide resolved
@havogt havogt requested a review from egparedes December 5, 2023 16:05
Copy link
Contributor

@egparedes egparedes left a 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.

src/gt4py/next/common.py Outdated Show resolved Hide resolved
src/gt4py/next/common.py Outdated Show resolved Hide resolved
src/gt4py/next/common.py Outdated Show resolved Hide resolved
src/gt4py/next/common.py Outdated Show resolved Hide resolved
src/gt4py/next/common.py Outdated Show resolved Hide resolved
src/gt4py/next/common.py Outdated Show resolved Hide resolved
src/gt4py/next/common.py Outdated Show resolved Hide resolved
src/gt4py/next/common.py Outdated Show resolved Hide resolved
tests/next_tests/unit_tests/test_common.py Show resolved Hide resolved
src/gt4py/next/common.py Outdated Show resolved Hide resolved
src/gt4py/next/common.py Outdated Show resolved Hide resolved
src/gt4py/next/common.py Outdated Show resolved Hide resolved
Comment on lines +210 to +212
return (self.start > other.start and self.stop <= other.stop) or (
self.start >= other.start and self.stop < other.stop
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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.

Copy link
Contributor Author

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

Copy link
Contributor

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.

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:
Copy link
Contributor

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?

Copy link
Contributor Author

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

@havogt havogt requested a review from tehrengruber December 7, 2023 15:48
Copy link
Contributor

@DropD DropD left a 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.

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.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
raise ValueError("Cannot compute length of open UnitRange.")
raise ValueError("Cannot compute length of open 'UnitRange'.")

Copy link
Contributor

@egparedes egparedes left a 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.

src/gt4py/next/common.py Outdated Show resolved Hide resolved
src/gt4py/next/common.py Show resolved Hide resolved
src/gt4py/next/common.py Outdated Show resolved Hide resolved
Comment on lines +210 to +212
return (self.start > other.start and self.stop <= other.stop) or (
self.start >= other.start and self.stop < other.stop
)
Copy link
Contributor

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 Show resolved Hide resolved
src/gt4py/next/common.py Outdated Show resolved Hide resolved
@@ -283,18 +341,21 @@ def named_range(v: tuple[Dimension, RangeLike]) -> NamedRange:
return (v[0], unit_range(v[1]))


_Rng = TypeVar("_Rng", bound=UnitRange)
Copy link
Contributor

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?

Copy link
Contributor Author

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],
)

@havogt
Copy link
Contributor Author

havogt commented Dec 18, 2023

@tehrengruber I will merge this PR. If you have more comments we can discuss them in a separate PR.

@havogt havogt merged commit cdcd653 into GridTools:main Dec 18, 2023
19 checks passed
@havogt havogt deleted the unit_range_comparison branch December 18, 2023 07:52
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.

4 participants