Base expression node types on dataclasses
#125
Conversation
|
Surprisingly, this seems to somewhat work. (Current CI notwithstanding) @kaushikcfd @alexfikl @isuruf What do you think? |
|
For context, this happened because I was working on inducer/pytato#393 and had a need to have the persistent hash key builder just "understand" expression values. Loopy has some absurd hack job to make this possible, but I wasn't itching to replicate it in pytato. This seemed nicer. |
inducer
changed the title
attrs classesattrs classes
inducer
changed the title
attrs classesattrs
|
Generally seems like a good idea to me! One thing that doesn't quite spark joy is having to remember which classes are |
|
I'm guessing I may revert this to dataclasses, for exactly this reason. Not sure. |
inducer
changed the title
attrsdataclasses
|
I've changed this to |
|
Most everything seems to be working, except pytential has a puzzling failure. |
There was a problem hiding this comment.
Looked a bit through the code to see if anything jumps out that could cause the pytential errors (in Beltrami stuff again 😢) and left some nitpicks!
The only thing (?) that the Beltrami operators do a bit differently is that they have nested IntG expressions, but I couldn't find any reason this would change how those are handled..
EDIT: Found a simpler case to run (uses includes from test_beltrami.py)
def test_nested_potential(actx_factory):
logging.basicConfig(level=logging.INFO)
actx = actx_factory()
case = eid.CircleTestCase(
target_order=5,
qbx_order=5,
source_ovsmp=4,
resolutions=[32, 64, 96, 128],
# FIXME: FMM should not be slower!
fmm_order=False, fmm_backend=None,
radius=1.0
)
from pytential import GeometryCollection
qbx = case.get_layer_potential(actx, 32, case.target_order)
places = GeometryCollection(qbx, auto_where=case.name)
from sumpy.kernel import LaplaceKernel
kernel = LaplaceKernel(2)
sym_sigma = sym.var("sigma")
sym_op = sym.D(kernel,
sym.D(kernel, sym_sigma, qbx_forced_limit="avg"),
qbx_forced_limit="avg")
density_discr = places.get_discretization(case.name)
sigma = actx.thaw(density_discr.nodes()[0])
r = bind(places, sym_op)(actx, sigma=sigma)
Tried S(Spp + Dp) and S(kappa * S) and S(W(S)) (the other terms in the Laplace-Beltrami operator) and the D(D) one was the only one that failed.
|
Thanks for finding the smaller reproducer! That helped quite a bit. The issue was the missing rewriting from |
|
Super weird. I wonder why the pytential run is getting canceled. Out of memory for some reason? If so, why would this PR make a difference? |
|
Identical PR on Gitlab: https://gitlab.tiker.net/inducer/pymbolic/-/merge_requests/64 |
|
Tried to investigate more. Here's the time profile of the pytential tests, first with this patch: and then without: (run on tripel, back to back with inducer/pytential@488b958 and inducer/sumpy@8960662) The differences are... interesting. I'm not sure I understand them. |
Ran this on to get the memory and it seems pretty similar as well (blue is on
Sooo.. really not sure why the github CI is crashing.. EDIT: Not sure how accurate this is, but Github reports having 7GB on their machines (and above we're using 12GB) |
|
In a way I think it's not a surprise that the runners get canceled. They have about 7G of memory available, plus 4G swap, so the 12G peak allocation is definitely flirting with disaster, and I think which one gets cancelled is probably just up to coincidence. Perhaps the more useful question here might be why the curve is monotonically increasing, when in reality all tests should be independent, and so there shouldn't be an increase. I propose we use inducer/sumpy#178 to have this discussion, because that looks eerily related. |
inducer
force-pushed
the
attrs
branch
8 times, most recently
from
83feb59 to
2340251
Compare
|
Some interesting (informal) timing data, using
All cases run with I'm thinking I will set |
No objection, I see similar performance numbers on my M1. My only concern is that setting Could a pattern such like this be helpful here? from dataclasses import dataclass, FrozenInstanceError
from typing import Any
@dataclass(frozen=False)
class Foo:
x: int
y: int
def __setattr__(self, name: str, value: Any) -> None:
if hasattr(self, "_frozen"):
raise FrozenInstanceError
object.__setattr__(self, name, value)
def __post_init__(self):
self._frozen = True
f = Foo(1, 2)
f.x = 42 # raises dataclasses.FrozenInstanceError
f.__setattr__("x", 42) # raises dataclasses.FrozenInstanceError
object.__setattr__(f, "x", 42) # worksPerhaps better: https://rednafi.com/python/statically_enforcing_frozen_dataclasses |
Have you tried the traversal benchmark with this? (I would expect it would tank it way more than leaving
The only scenario leading to confusion IMO is someone who only tests code during development with
That's effectively already what's going on: @dataclass_transform(frozen_default=True)
def expr_dataclass(init: bool = True) -> Callable[[type[_T]], type[_T]]:
... |
You're right, it is slower than setting
I see, thanks! I was not aware of this feature. |
Matthias Diener contributed significantly to this by figuring out that innumerable warnings led to out-of-memory situations with pytest. Co-authored-by: Matthias Diener <[email protected]>
Stacking @classmethod, @Property is banned in Python 3.13.
Closes #46.