Fix yet two more bugs#103
Conversation
| run: | | ||
| cd ./tests/code-gen | ||
| for example in matmul minimal indices slicing; do | ||
| for example in matmul minimal indices slicing bugfixing; do |
There was a problem hiding this comment.
The term "bugfixing" does not really say anything. Could you find a better name for this, please?
There was a problem hiding this comment.
In this case, it's meant to have test cases that would've failed before—i.e. bugs that were fixed (hence also the PR number where they were fixed as comments). Not sure how to name it better; maybe fixed_bugs?
There was a problem hiding this comment.
Since it is more about testing for earlier bugs, I would include something that clarifies this is testing for older bugs. Something like testing_old_bugs or a shorter alternative. I don't like what I suggested because it is too long, but you could choose something comprehensive.
There was a problem hiding this comment.
Ok; renamed it to regress; as in "regression tests". But not regression as that might sound a bit too much like a math regression problem that the test would implement IMO.
There was a problem hiding this comment.
Ok; renamed it to
regress; as in "regression tests". But notregressionas that might sound a bit too much like a math regression problem that the test would implement IMO.
Sure, the name is still not fully clear, but I don't have a better idea myself. You could keep it.
| def visit_Add(self, node): | ||
| variables = [self.visit(child) for child in node] | ||
| assert len(variables) > 1 | ||
| assert len(variables) >= 1 |
There was a problem hiding this comment.
Just to clarify, could you please elaborate a bit on this change?
There was a problem hiding this comment.
The computation also works for length 1 without adjustments (but not so for 0 I think, but I haven't tried it); so all we do here is make the assert more lenient for that exact case.
(it doesn't quite return the original tensor here; but will still generate a copy-scale-add operation—though that one should be, IIRC, optimized away or merged with other operations afterwards)
There was a problem hiding this comment.
How about we verify that it does not work for 0 first? If it also works for 0, maybe you could remove the assert.
There was a problem hiding this comment.
I tried it—it seems to fail on some more fronts (i.e. it takes longer than 10 mins to fix it).
2 participants
result <= Add() + term)