Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Pytorch support for Join and Careduce Ops #869
Pytorch support for Join and Careduce Ops #869
Changes from 8 commits
48d16ff
a8c1fb1
14dde44
0bdeb2e
bbb3622
519fd47
9073a56
036082b
8dfa059
18bbc8c
35153f7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 44 in pytensor/link/pytorch/dispatch/elemwise.py
Codecov / codecov/patch
pytensor/link/pytorch/dispatch/elemwise.py#L44
Check warning on line 54 in pytensor/link/pytorch/dispatch/elemwise.py
Codecov / codecov/patch
pytensor/link/pytorch/dispatch/elemwise.py#L54
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 axis[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.
op.axis
is a tuple, pytorch expects integers fordim
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.
Then we need to change the logic, because it is possible for them to be tuples with more than one entry
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.
We could do something like this:
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.
If you reduce in reversed order you don't have to worry about the keepdims. Sounds good, a bit surprising that they don't support multiple axes
Check warning on line 64 in pytensor/link/pytorch/dispatch/elemwise.py
Codecov / codecov/patch
pytensor/link/pytorch/dispatch/elemwise.py#L64
Check warning on line 74 in pytensor/link/pytorch/dispatch/elemwise.py
Codecov / codecov/patch
pytensor/link/pytorch/dispatch/elemwise.py#L74
Check warning on line 84 in pytensor/link/pytorch/dispatch/elemwise.py
Codecov / codecov/patch
pytensor/link/pytorch/dispatch/elemwise.py#L84
Check warning on line 94 in pytensor/link/pytorch/dispatch/elemwise.py
Codecov / codecov/patch
pytensor/link/pytorch/dispatch/elemwise.py#L94
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.
Can we parametrize these tests with the reduce function? Since they all look the same, we can reduce a bunch of lines. Or at least separate only those that need numerical inputs from those that need boolean (all and any).
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.
Also I would like to test axis = (1, 2), and have
a_pt
be a tensor3, so that we cover the case with more than 1 axis, but not all of them.