-
Notifications
You must be signed in to change notification settings - Fork 106
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Canonicalize Subtensor slices #761
Conversation
For all the tests that are failing ( eg when I do
However this occurs because of Similarly for What do these registers do and why do the changes that I have made force them to fail? |
Assume
This happens because the shape of input How can I make this rewrite: |
The error message tells you there's likely a bug in your new rewrite. The returned variable should have the same type as the original one. You can print node.outputs[0].type to see the original type and print the type of your proposed replacement. Perhaps you lost track of some non slice indexes during the rewrite? |
|
Your /other rewrite shouldn't need to be aware of each other, because each should leave the graph consistent with how it was before. |
Do you have a test that is failing that we can look at? |
This is a dummy test. The assert will be replaced with equal_computations. |
The first failing test in the CI, prints:
The rewrite is trying to replace a length 1 vector by a scalar. Stepping into the debugger with a breakpoint on the new rewrite may help you figure out where the bug is |
6e84a34
to
36645b7
Compare
So for the recent changes, tests like
I tried printing the
With this rewrite:
So how can I preserve this trace in my rewrite? |
Check the utility |
Is it always necessary to have Should I proceed to drop this assert in the rewrite as it seems to be flexible? |
The returned type must be compatible with the original type that it is substituting. That means the same number of dimensions and compatible shape info. You can replace something of type.shape=(5, None) by something with type.shape (None, 3) and vice-versa, but not something of type.shape=(5, None) by something with type.shape=(3, None) or (5,) (less dims) |
I've noticed Scan rewrites being even more strict about this, would be good to check which line is being too strict in the failing tests. |
So according to that, (3, None) and (5, None) will also not be compatible right? This can be done simple by checking if it is preserved or is now none, right? |
Sorry, I missed your |
That's great! Then I can drop the |
863105a
to
a681a0e
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #761 +/- ##
=======================================
Coverage 81.74% 81.75%
=======================================
Files 183 183
Lines 47733 47749 +16
Branches 11616 11620 +4
=======================================
+ Hits 39020 39037 +17
+ Misses 6518 6517 -1
Partials 2195 2195
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great, some more suggestions for optimization and combining with the existing rewerite
8833ca5
to
edc5ddd
Compare
edc5ddd
to
34b084f
Compare
c1a9f26
to
d681749
Compare
# Test case 1 | ||
y = x[0:None, 0:5, 0:7, 0:9:1] | ||
f = pytensor.function([x], y) | ||
test_y = f.maker.fgraph.outputs[0].owner.inputs[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not the output directly, there's a deepcopy?
d681749
to
4271d58
Compare
@ricardoV94 @Dhruvanshu-Joshi Is there anything left preventing this PR from being merge? |
I had a small question above, why we are checking the input of the output instead of the output directly |
Hi @ricardoV94 @HangenYuu , you mean |
Then it's good to assert that as well so everything is explicit |
Can we eliminate the deepcopy using assert? Sounds great, how can we do so? |
I didn't mean to eliminate, I meant asserting that the Op of that fist node is a DeepCopy. That will make the test less mysterious. Alternative you can say that you expect a deepcopy around the slice in |
4271d58
to
96c00f7
Compare
Hi @ricardoV94 Here's my log of running
What should I do to make the tests pass? |
96c00f7
to
af7f205
Compare
af7f205
to
0f6a7a0
Compare
Hi @ricardoV94 |
That was all thanks for the reminder @Dhruvanshu-Joshi :) |
Description
Herein I aim to convert all:
Related Issue
Subtensor
slices #58Checklist
Type of change