search

sympy / sympy_benchmarks

Some benchmarks of SymPy
14 stars 32 forks source link

Add new CSE benchmark based on Kalman filter matrix expression #83

Closed brocksam closed 1 year ago

brocksam commented 1 year ago

Issue https://github.com/sympy/sympy/issues/24696 highlighted that CSE on matrix expressions was extremely slow because terms were eagerly evaluated during the optimisation pass. PR https://github.com/sympy/sympy/issues/24696 implemented a fix that reduces the runtime on this 2x2 example by ~10,000x, even more for larger systems. It's worth adding as a benchmark to avoid performance regressions in the future.

brocksam commented 1 year ago

Unsure whether this should be treated as a "slow" benchmark or not. Runtime is approximately ~60s. However, https://github.com/sympy/sympy/issues/24696 reduces it to ~6ms. So in the historic case it's "slow", but moving forward it'll be fast.

asmeurer commented 1 year ago

I think asv will timeout on this for the 60s version by default. I guess it just depends on whether we want to have a correct historical graph for this. Personally I care much more about the benchmarks as a regression detection, so I would leave this as something that always runs.

brocksam commented 1 year ago

In that case I think this is good to go from my end then.

moorepants commented 1 year ago

Should we wait to merge after your fix goes it? (so it doesn't just add 60 sec to the PR CI)

brocksam commented 1 year ago

Should we wait to merge after your fix goes it? (so it doesn't just add 60 sec to the PR CI)

Yes, should probably wait. https://github.com/sympy/sympy/issues/24696 is ready so and appears approved by reviewers so just waiting for it to be merged, hopefully soon.

asmeurer commented 1 year ago

Should this be merged now?

brocksam commented 1 year ago

I think so, yes please.