Add support for lists for prim::min and prim::max#26155
Add support for lists for prim::min and prim::max#26155Krovatkin wants to merge 2 commits intopytorch:masterfrom
Conversation
eellison
left a comment
There was a problem hiding this comment.
Looks good, couple comments
There was a problem hiding this comment.
nit: combine test_min_list and test_max_list and then rewrite inputs as a pairwise iteration over a list of inputs ? would be much shorter
There was a problem hiding this comment.
it's a little tricky since I have empty-list tests and those will get typed as Tensor[]. Let me see if I can make it look a bit better.
There was a problem hiding this comment.
I don't think they will get typed as Tensor[] since they're inputs to the function
There was a problem hiding this comment.
It would be nice if there was a way to write minList and maxList as one function, since the only difference is the comparator... i dont think theres a nice way of doing it though
There was a problem hiding this comment.
I think there's a way: multiplying by -1 both sides is equivalent to switching from < to >, but I don't think it's worthwhile to deduplicate these two functions at the expense of readability. And I hope we won't hit a bug with these, since min/max of lists is a fairly canonical problem to the point everyone just remembers it by heart.
There was a problem hiding this comment.
thx!
edit: actually - i thought you were adding supporting for max(list) but you're adding support for max(list1, list2) (which is also good). since you are doing the latter not the form do you mind dropping this? sorry for confusion.
fix prim::int(int[] x) case when x = [] refactor tests merge min/max tests remove 0-length prim::min support fix clang-tidy errors add bools case back
2dfbc6c to
5596528
Compare
facebook-github-bot
left a comment
There was a problem hiding this comment.
@Krovatkin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@Krovatkin merged this pull request in 18eb92e. |
Summary: Pull Request resolved: pytorch#26155 Differential Revision: D17455540 Pulled By: Krovatkin fbshipit-source-id: e3aee465d108b59691d6c68f85fbf212a5d6a125
No description provided.