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.

Sign up for GitHub

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

Jump to bottom

Add explicit scopes to CSE #150

Merged
merged 4 commits into from
Nov 9, 2024
Merged

Add explicit scopes to CSE #150

merged 4 commits into from
Nov 9, 2024

Conversation

alexfikl
Copy link
Collaborator

@alexfikl alexfikl commented Oct 1, 2024

There are quite a few deprecation warnings from that, so this adds the explicit scopes in mappers and others things.

I didn't add it in make_common_subexpression. Should that also defaults to EVALUATION now? As a "helper" function, I would sort of expect it to do the right thing™ (?).

inducer
inducer reviewed Oct 1, 2024
View reviewed changes
Copy link
Owner

@inducer inducer left a comment

Choose a reason for hiding this comment

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

Thx!

pymbolic/mapper/c_code.py Outdated Show resolved Hide resolved
pymbolic/primitives.py Outdated
Comment on lines 902 to 903
warn("Attribute 'init_arg_names' of {cls} is deprecated and will "
"be removed in 2025. Use dataclasses.fields instead.",
Copy link
Owner

Choose a reason for hiding this comment

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

The phrasing here is a bit odd, since it claims we'll magically remove these attributes in 2025. Reword?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tried to reword it a bit in https://github.com/inducer/pymbolic/compare/9a62b35b5063d40cdbbb2084414e124141f3406b..7eba5378d83bfd5d6146ed9a94f16478758a801a. Let me know if that sounds clearer!

pymbolic/primitives.py Outdated
Comment on lines 916 to 917
warn("Method '__getinitargs__' of {cls} is deprecated and will "
"be removed in 2025. Use dataclasses.fields instead.",
Copy link
Owner

Choose a reason for hiding this comment

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

The phrasing here is a bit odd, since it claims we'll magically remove these attributes in 2025. Reword?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reworded in https://github.com/inducer/pymbolic/compare/9a62b35b5063d40cdbbb2084414e124141f3406b..7eba5378d83bfd5d6146ed9a94f16478758a801a.

pymbolic/primitives.py Outdated Show resolved Hide resolved
pymbolic/primitives.py Outdated
@@ -1796,13 +1796,13 @@ def wrap_in_cse(expr, prefix=None):
if prefix is None:
return expr
if expr.prefix is None and type(expr) is CommonSubexpression:
Copy link
Owner

Choose a reason for hiding this comment

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

Use same logic as make_cse below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hm, I'm not sure what you had in mind here? At least at a first glance, these seem to have different logic: make_cse ignores the prefix when re-wrapping and wrap_in_cse ignores the scope when re-wrapping.

Copy link
Owner

Choose a reason for hiding this comment

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

To be honest, I'm not sure why we have both of these, and I think the differences are mostly spurious.

The one actual difference seems to be that Variable and Subscript aren't re-wrapped, but that could be a flag on make_common_subexpression, at which point wrap_in_cse could be deprecated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

wrap_in_cse seems to be exclusively used in sumpy, so I'm guessing it needed a fast re-wrap thing at some point.

I went ahead and deprecated it and replaced it with a flag in make_common_subexpression in
https://github.com/inducer/pymbolic/compare/3bcf915c6b2a360eb09b98c7aba4d2641cfccf4e..451d51fe3936d7d157de7ab766d7a0d1bda65518

test/test_pymbolic.py Outdated Show resolved Hide resolved
alexfikl
alexfikl commented Oct 2, 2024
View reviewed changes
pymbolic/mapper/c_code.py Outdated
Comment on lines 54 to 55
Notice that if we were to directly generate code from this, the
subexpression *u* would be evaluated multiple times.
subexpression *u* would not be evaluated multiple times.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

? Is this right?

Copy link
Owner

Choose a reason for hiding this comment

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

By "directly", it means "without CSEs". Rephrase, bring the "not" back?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, I see! Reworded a bit more verbosely in
https://github.com/inducer/pymbolic/compare/3bcf915c6b2a360eb09b98c7aba4d2641cfccf4e..451d51fe3936d7d157de7ab766d7a0d1bda65518

alexfikl
alexfikl commented Oct 2, 2024
View reviewed changes
pymbolic/primitives.py Outdated Show resolved Hide resolved
alexfikl
alexfikl commented Oct 6, 2024
View reviewed changes
pymbolic/primitives.py Outdated
Comment on lines 927 to 954
cls.__eq__ = {cls.__name__}_eq
if {hash}:
cls.__eq__ = {cls.__name__}_eq
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This also guards the __eq__ override with hash. Was playing with IntG and it also needs to overwrite equality (seems mostly used in tests?).

Not sure if it's worth adding a separate eq flag (to match dataclass)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Would these better default to "__eq__" in cls.__dict__? It seems like the dataclass decorator also does similar checks to not overwrite the user implementation.

Copy link
Owner

Choose a reason for hiding this comment

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

  • I think a separate eq flag makes sense.
  • I agree that, by default, user implemenations should not be overwritten.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed! Made those changes in
https://github.com/inducer/pymbolic/compare/3bcf915c6b2a360eb09b98c7aba4d2641cfccf4e..451d51fe3936d7d157de7ab766d7a0d1bda65518

@alexfikl alexfikl mentioned this pull request Oct 9, 2024
Merged
3 tasks
@alexfikl alexfikl force-pushed the dataclass-extras branch 2 times, most recently from 298c11b to 3ff2b5d Compare November 7, 2024 14:24
@inducer inducer merged commit 670bbbb into inducer:main Nov 9, 2024
11 checks passed
@inducer
Copy link
Owner

inducer commented Nov 9, 2024

Thanks!

@inducer
Copy link
Owner

inducer commented Nov 9, 2024

This was the last thing missing for 2024.1. Releasing that now.

cc @matthiasdiener

@alexfikl alexfikl deleted the dataclass-extras branch November 9, 2024 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@inducer inducer inducer left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@alexfikl @inducer