inducer / pymbolic

A simple package to do symbolic math (focus on code gen and DSLs)
http://mathema.tician.de/software/pymbolic
Other
106 stars 25 forks source link

Add EqualityMapper #74

Open alexfikl opened 2 years ago

alexfikl commented 2 years ago

Added an EqualityMapper similar to the one in inducer/pytato#166.

This seems a bit slower than the current version (benchmarked against inducer/pytential#121): it went from 5.3s-ish to 6.6s-ish.

Fixes #73.

inducer commented 2 years ago

@kaushikcfd Is this worth pursuing given that compile times are suffering because of expression traversal complexity in pymbolic?

alexfikl commented 2 years ago

@kaushikcfd Is this worth pursuing given that compile times are suffering because of expression traversal complexity in pymbolic?

For what it's worth, the loopy branch is up to date and passing tests locally, so it shouldn't be too hard to clean this up.

kaushikcfd commented 2 years ago

Is this worth pursuing given that compile times are suffering because of expression traversal complexity in pymbolic?

I think this is useful. IIUC, this PR quite significantly brings down the complexity of the expression comparison. If the overheads of this approach (creating a new mapper for every comparison) are under-control, I would love to get rid of hacks such as: https://github.com/kaushikcfd/pymbolic/blob/a697d47cd53e9902abc49f98d67921b466c992c2/pymbolic/mapper/__init__.py#L213.

kaushikcfd commented 2 years ago

However, I wonder how the loopy CI failures could be fixed? By creating a derived version of every pymbolic expression type in loopy so that they can use loopy-specific EqualityMapper?

alexfikl commented 2 years ago

However, I wonder how the loopy CI failures could be fixed? By creating a derived version of every pymbolic expression type in loopy so that they can use loopy-specific EqualityMapper?

It should work with the modified branches. I imagine it needs some work in ci-support.sh to clone the correct repo and branch (instead of hardcoding to inducer/loopy@main).

kaushikcfd commented 2 years ago

Oops had missed the branch in the description. I think those will work fine. Thanks!

inducer commented 2 years ago

I would love to get rid of hacks such as:

Whoa. I had missed that during review. That is pretty gross.

inducer commented 2 years ago

Add a test like https://github.com/kaushikcfd/pytato/blob/1d315637ee237631b30a24228375c33257ea7dbe/test/test_pytato.py#L307-L330

inducer commented 2 years ago

I need to do some measuring.