Add explicit scopes to CSE #150
Conversation
pymbolic/primitives.py
Outdated
| 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!
pymbolic/primitives.py
Outdated
| 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.
alexfikl
force-pushed
the
dataclass-extras
branch
from
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. |
|
|
||
| def make_common_subexpression(expr: ExpressionT, | ||
| prefix: str | None = None, | ||
| scope: str | None = None) -> ExpressionT: |
There was a problem hiding this comment.
Left scope=None here because setting it to scope=cse_scope.EVALUATION shows the actual string in the docs (i.e. "pymbolic_eval"), which are not shown in the docs for cse_scope itself.. very confusing :\
alexfikl
force-pushed
the
dataclass-extras
branch
from
7eba537
to
4c6bda8
Compare
inducer
force-pushed
the
dataclass-extras
branch
from
4c6bda8
to
76aaa11
Compare
alexfikl
force-pushed
the
dataclass-extras
branch
from
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.
alexfikl
force-pushed
the
dataclass-extras
branch
from
713921b
to
8c70f5e
Compare
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™ (?).