pymbolic icon indicating copy to clipboard operation
pymbolic copied to clipboard

Add EqualityMapper

Open alexfikl opened this issue 4 years ago • 9 comments

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.

  • [x] Rebase once #72 is in to remove extra commits.
  • [x] Need to figure out why it's so slow (not caching properly?).
  • [x] Add test that this is actually caching.
  • [x] Add benchmark of some sort that it actually makes things better in some pathological cases?
  • [ ] Update and pass tests in downstreams. Current work at
    • [ ] https://github.com/inducer/loopy/pull/607
    • [ ] https://github.com/inducer/pytential/pull/148
    • [ ] https://github.com/inducer/pytato/pull/317
    • [x] https://github.com/inducer/grudge/pull/255
  • [ ] Point CI back to the proper downstream branches.
  • [ ] Remove fail-fast from downstream CI

Fixes #73.

alexfikl avatar Nov 15 '21 16:11 alexfikl

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

inducer avatar Apr 28 '22 16:04 inducer

@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.

alexfikl avatar Apr 28 '22 16:04 alexfikl

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 avatar Apr 28 '22 17:04 kaushikcfd

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?

kaushikcfd avatar Apr 28 '22 17:04 kaushikcfd

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).

alexfikl avatar Apr 28 '22 17:04 alexfikl

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

kaushikcfd avatar Apr 28 '22 17:04 kaushikcfd

I would love to get rid of hacks such as:

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

inducer avatar Apr 29 '22 00:04 inducer

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

inducer avatar Apr 29 '22 22:04 inducer

I need to do some measuring.

inducer avatar Apr 29 '22 22:04 inducer