Add explicit scopes to CSE - #150
Conversation
| warn("Attribute 'init_arg_names' of {cls} is deprecated and will " | ||
| "be removed in 2025. Use dataclasses.fields instead.", |
There was a problem hiding this comment.
The phrasing here is a bit odd, since it claims we'll magically remove these attributes in 2025. Reword?
There was a problem hiding this comment.
Tried to reword it a bit in https://github.com/inducer/pymbolic/compare/9a62b35b5063d40cdbbb2084414e124141f3406b..7eba5378d83bfd5d6146ed9a94f16478758a801a. Let me know if that sounds clearer!
| warn("Method '__getinitargs__' of {cls} is deprecated and will " | ||
| "be removed in 2025. Use dataclasses.fields instead.", |
There was a problem hiding this comment.
The phrasing here is a bit odd, since it claims we'll magically remove these attributes in 2025. Reword?
There was a problem hiding this comment.
| @@ -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: | |||
There was a problem hiding this comment.
Use same logic as make_cse below.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
9a62b35 to
7eba537
Compare
| 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. |
There was a problem hiding this comment.
By "directly", it means "without CSEs". Rephrase, bring the "not" back?
There was a problem hiding this comment.
Ah, I see! Reworded a bit more verbosely in
https://github.com/inducer/pymbolic/compare/3bcf915c6b2a360eb09b98c7aba4d2641cfccf4e..451d51fe3936d7d157de7ab766d7a0d1bda65518
7eba537 to
4c6bda8
Compare
4c6bda8 to
76aaa11
Compare
76aaa11 to
ba97f09
Compare
|
|
||
| cls.__eq__ = {cls.__name__}_eq | ||
| if {hash}: | ||
| cls.__eq__ = {cls.__name__}_eq |
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
- I think a separate
eqflag makes sense. - I agree that, by default, user implemenations should not be overwritten.
There was a problem hiding this comment.
713921b to
8c70f5e
Compare
298c11b to
3ff2b5d
Compare
3ff2b5d to
3bcf915
Compare
3bcf915 to
451d51f
Compare
|
Thanks! |
|
This was the last thing missing for 2024.1. Releasing that now. |
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 toEVALUATIONnow? As a "helper" function, I would sort of expect it to do the right thing™ (?).