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. |
|
OK, so inducer/sumpy#178 seems to be a different issue. |
|
Made inducer/pytential#213 to discuss the unbounded memory growth. |
|
Just wanted to write this down before I forget it again! After a run on uses
so that may be a good candidate to look at to see what's ballooning the memory. |
|
Also slightly related: tried hacking in |
No description provided.