Make F.rotate/F.affine accept learnable params #5110

vfdev-5 · 2021-12-17T15:33:33Z

Following #4995 (comment) and that PR,
here is

F.rotate
F.affine
that should be able to accept differentiable params and also be jit scriptable.

@ain-soph we can try to merge this PR at first and then you can update your PR based on this one

…todiff

facebook-github-bot · 2021-12-17T15:33:39Z

💊 CI failures summary and remediations

As of commit 912dafb (more details on the Dr. CI page):

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

ain-soph · 2021-12-18T17:22:54Z

Thanks a lot! I'll follow your style to work on other methods.

vadimkantorov · 2021-12-19T20:45:27Z

torchvision/transforms/functional.py

+
+    # RSS without scaling
+    sx, sy = shear_rad[0], shear_rad[1]
+    a = torch.cos(rot - sy) / torch.cos(sy)


just wondering, could here be division by zero?

Yes, sy = pi/2 and same for sx=pi/2 with tan. IMO shear of ~90 degrees is too much distortion

Maybe best to just protect against division by exact zero, or point it out in docs

Docs however say that values between -180 and 180 are valid. I'm not sure how can we protect from zero devision if user requires 90 degree, like how to evaluate tan(pi/2) without getting infinity. Any ideas ?

I don't think it should be solved in this issue since original non-tensor codes also have this problem.
Maybe we can open a new issue about this. The potential solution might be just to modify the docs or clip within a range. 90 degree shearing seems meaningless.

Yes, if it's not guarded against, the docs or a separate issue would be great.

@vadimkantorov please create a new issue for this discussion. Thanks

torchvision/transforms/functional.py

vfdev-5 · 2021-12-22T08:49:31Z

test/test_transforms_tensor.py

@@ -464,6 +464,7 @@ def test_resized_crop_save(self, tmpdir):


 def _test_random_affine_helper(device, **kwargs):
+    torch.manual_seed(12)


I had to add this line and fix shear value due to some random jit vs non-jit results mismatch in one pixel

ain-soph · 2022-06-29T03:51:24Z

Hi, @vfdev-5, sorry for being busy during the past semester. I'm currently available in the following month.
I want to know where we are right now and make contribution to this PR.

Is there any problem to merge the current PR? If not, I'll draft the PR for other operators.

datumbox · 2022-06-29T10:05:51Z

We might need to hold off this PR until the situation on Transforms API v2 is clearer. I think the two can be aligned but to avoid BC breaking changes we need to think them carefully. @vfdev-5 any concerns?

ain-soph · 2022-06-29T22:30:58Z

@datumbox (Maybe a question not that relevant) I'm currently doing internship at Meta ([email protected]), will that help for making contribution and communicate ideas for the issue?

The basic idea for this PR is to wrap parameters with torch.Tensor instead of plain float to allow gradient computation w.r.t. those parameters, so I personally don't see anything that breaks backward compatibility yet.

datumbox · 2022-06-30T08:52:24Z

@ain-soph Thanks for the feedback! I've pinged you via the internal chat but for transparency and to keep the community up-to-date I'll summarize why I proposed to wait a little bit.

The current prototype implementation merged on main branch doesn't have the F layer. This high-level layer of functional transforms, which is also the public interface for the users, connects the low-level kernels (PIL, Tensor). In the original new proposal, this layer was removed in favour of exposing directly the low-level kernels. This was done because the new solution uses Tensor subclassing that is NOT JIT-scriptable. So merging the PR means we are doing changes on an API that could be deprecated. Nevertheless, we are currently examining introducing it back to resolve the verbosity of the proposed API on the Transforms. @vfdev-5 has actually an open PR for that at #6219. So waiting a little bit longer to help us finalize the API will allow us to make better decisions. This doesn't mean that we ignore your feature request, just postpone the merge for a bit until the API proposal is finalized. Let me know if you have more concerns, happy to schedule a chat.

vfdev-5 · 2022-06-30T09:37:21Z

@datumbox at first we can apply this PR's logic to proto low-level ops, e.g. F.rotate -> F.rotate_image_tensor. Next, we can try to apply that to proto's F.rotate. Here i tried to apply that for rotate op: main...vfdev-5:vision:proto-learnable-params
I still have some test issues, but I think this can be fixed.

ain-soph · 2023-05-14T22:00:02Z

@vfdev-5 @datumbox Would we reconsider this PR since Transfroms V2 API is out?

vfdev-5 added 2 commits December 7, 2021 13:39

WIP on differentiable F.rotate

1214e6f

Merge branch 'main' of github.com:pytorch/vision into check-rotate-au…

745ee27

…todiff

pytorch-probot bot added the ciflow/default label Dec 17, 2021

facebook-github-bot added the cla signed label Dec 17, 2021

JIT should be happy now

eb826c3

vfdev-5 force-pushed the check-rotate-autodiff branch from 5bcdd0d to eb826c3 Compare December 17, 2021 15:45

Removed unused import

447d549

vfdev-5 requested a review from datumbox December 17, 2021 16:56

vfdev-5 changed the title ~~Make F.rotate accept learnable params~~ Make F.rotate/F.affine accept learnable params Dec 17, 2021

vfdev-5 marked this pull request as draft December 17, 2021 18:06

vadimkantorov reviewed Dec 19, 2021

View reviewed changes

torchvision/transforms/functional.py Outdated Show resolved Hide resolved

ain-soph mentioned this pull request Dec 19, 2021

Make transforms.functional_tensor functions differential w.r.t. their parameters #4995

Closed

6 tasks

vfdev-5 added 4 commits December 21, 2021 16:58

Added support for learnable params to affine ops and refactored the code

990dfdf

Added tests checking raises error for non-float input

ac199e4

Re-enabled _assert_grid_transform_inputs

7bd8d65

Merge branch 'main' into check-rotate-autodiff

912dafb

vfdev-5 marked this pull request as ready for review December 22, 2021 08:48

vfdev-5 commented Dec 22, 2021

View reviewed changes

vfdev-5 mentioned this pull request Jan 4, 2022

[RFC] Make transforms.functional methods differential w.r.t. their parameters #5157

Open

12 tasks

vfdev-5 changed the base branch from main to learnable_params January 5, 2022 15:41

		@@ -464,6 +464,7 @@ def test_resized_crop_save(self, tmpdir):


		def _test_random_affine_helper(device, **kwargs):
		torch.manual_seed(12)

Make F.rotate/F.affine accept learnable params #5110

Are you sure you want to change the base?

Make F.rotate/F.affine accept learnable params #5110

Uh oh!

Conversation

vfdev-5 commented Dec 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Dec 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

Uh oh!

ain-soph commented Dec 18, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ain-soph commented Jun 29, 2022

Uh oh!

datumbox commented Jun 29, 2022

Uh oh!

ain-soph commented Jun 29, 2022

Uh oh!

datumbox commented Jun 30, 2022

Uh oh!

vfdev-5 commented Jun 30, 2022

Uh oh!

ain-soph commented May 14, 2023

Uh oh!

Uh oh!

vfdev-5 commented Dec 17, 2021 •

edited

Loading

facebook-github-bot commented Dec 17, 2021 •

edited

Loading