-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
Add torch frontend grid_sample and test function #22539
Conversation
Thanks for contributing to Ivy! 😊👏 |
Hi @Sam-Armstrong The issues was happened because I used an ivy boolean array to mask the grid. It will throw an error when the grid input is a torch Tensor, not ivy array. |
Sorry, here's a better example that's working:
I don't understand why grid is a torch.Tensor at that point though, shouldn't it have been converted into an ivy array? |
Hi @Sam-Armstrong So surprise to have you around this early :) Do you think this is a bug on @to_ivy_arrays_and_back decorator? Or should I covert them to ivy array in manually? |
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 can actually resolve that problem by fixing the minimal example to pass ivy arrays, so we can (and should) remove that ivy.native_array line. I've tried one of the test cases which is failing in pytest, and it seems to be passing fine in the minimal example 🤔 so I'll talk to someone from the testing team about that to see if they know what's going on. By the way, do you know why Exception occured: list index out of range
is printed out several times when running this? Thanks!
for i in range(1, len(borders)): | ||
zeros_mask = ivy.bitwise_or(zeros_mask, masks[i]) | ||
|
||
zeros_mask = ivy.native_array(zeros_mask) |
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.
zeros_mask = ivy.native_array(zeros_mask) |
We can remove this now
@@ -95,6 +95,7 @@ def affine_grid(theta, size, align_corners=False): | |||
return grid.view((N, D, H, W, 3)) | |||
|
|||
|
|||
# @with_unsupported_dtypes({"2.0.1 and below": ("float16", "bfloat16")}, "torch") |
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.
# @with_unsupported_dtypes({"2.0.1 and below": ("float16", "bfloat16")}, "torch") |
frac_in = ivy.abs(x / span) | ||
extra = (frac_in - ivy.floor(frac_in)) * ivy.abs(span) | ||
flips = ivy.floor(x / span) | ||
# x *= 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.
# x *= 0 |
Corrected minimal example:
|
Afternoon @Sam-Armstrong I just added the request change. Would you like share to more details about the test cases that gave you Update: Looks like this is because of PyTorch grid_sample() uses upper round, so they seems to have different behaviour when the coordinate is a x.5 values. Applying a ceiling on all x.5 values seems fix this issues. |
Hi @Sam-Armstrong Hope you are doing well this week. Did you get update from test team? From this week, I would need to move most of my effort to graduation. I might response slower than usual, but will try my best on get this PR done ;) |
I think @sherry30 is looking into this, not sure if there is any update? No worries at all, best of luck with your graduation work also @Deepdive543443 😁 |
Hi @Deepdive543443 @Sam-Armstrong ,
|
Hi @sherry30 @Sam-Armstrong After adding copy and type casting I think the function could pass three of the test for now. The remain issues are:
It looks like not just my implementation of grid_sample(), some others test function under |
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.
Thanks @Deepdive543443, that's great! I don't think these test errors are too much of a big deal - the other backends clearly show that the function is working correctly, so I think we can address these failures later. I've left a couple of comments with things we need to change before we merge, please could you take a look? And thanks for the great work!
ivy/functional/frontends/torch/nn/functional/vision_functions.py
Outdated
Show resolved
Hide resolved
ivy/functional/frontends/torch/nn/functional/vision_functions.py
Outdated
Show resolved
Hide resolved
ivy/functional/frontends/torch/nn/functional/vision_functions.py
Outdated
Show resolved
Hide resolved
Thank you @Sam-Armstrong and @sherry30 for supervising and guiding. I still not sure about if pre_commit is working correctly, so I placed all functions to the bottom of file after formatting. Hopefully it's good to go :) |
@Deepdive543443 I'll just check whether the tf and paddle test failures are ok with Sherry (rather than needing to fix them for this PR), then should be good to merge 😁 |
Merged! Thanks for the great work you put into this PR @Deepdive543443! |
Thanks @Sam-Armstrong and @sherry30 Finally it is done. Learned a lot from this task :) |
Co-authored-by: Sam Armstrong <88863522+Sam-Armstrong@users.noreply.github.com>
Close #22307