Create helper to standardize rewrite tests#2103
Conversation
e906a7b to
86aded4
Compare
|
Why do you think a numerical test should be standard after a structural comparison? It seems like the structural should be sufficient in most cases. |
I also like structural check but we've been bit by that in the past. We got the structure we expected but there was a flaw in it. The numbers are less likely to be fooled |
|
Example the square(sqrt) rewrite we got the wrong order recently. Needed nan in the test but you get the idea |
|
can we have this? :D |
Yes, may want to trim away stuff we're not sure to need, and also think a bit about the default rewrites we want, maybe nothing by default? And we can help and if a node_rewriter is passed, wrap it in dfs ourselves? |
ace9287 to
e3e3475
Compare
|
@jessegrabowski cleaned up and ready for review |
Provides RewriteTester class and rewrite_test factory that clone a graph, apply rewrites to the clone, and offer structural/numerical assertions without compiling full-mode functions.
e3e3475 to
600a8d6
Compare
This PR adds a helper to streamline our rewrite tests, according to what I these days feel is the best strategy:
For instance, the test in #2101 would look like:
I really don't like op counts, because it can miss stuff like AdvancedSubtensor become AdvancedSubtensor1, or Blockwise(Dot) become Dot, or Gemm, and it seems like we are optimizing stuff when we are not. Still I added the helpers to count ops...
The main goal though is to reduce friction / have a baseline for contributions