Skip to content
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 transpose_kernel argument to ConvTranspose. #2578

Merged
merged 1 commit into from
Jan 24, 2023

Conversation

mathisgerdes
Copy link
Contributor

This is a proposal for #2577, adding a transpose_kernel option to ConvTranspose such that the previous behavior is maintained by default.

  • Add an additional transpose_kernel parameter to ConvTranspose which is passed to jax.lax.conv_transpose
  • For padding='CIRCULAR': To maintain the previous convention of ConvTranspose, the alignment of the kernel is changed depending on transpose_kernel.
    # Divide the padding equally between left and right. The choice to put
    # "+1" on the left (or on the right) represents a convention for
    # aligning even-sized kernels.
    if self.transpose_kernel:
      # match convention in Conv to achieve the proper transpose
      total_pad = [
         (size_diff // 2, (size_diff + 1) // 2) for size_diff in size_diffs
      ]
    else:
      # keep the previous convention for backward compatibility
      total_pad = [
         ((size_diff + 1) // 2, size_diff // 2) for size_diff in size_diffs
      ]

@codecov-commenter
Copy link

codecov-commenter commented Nov 2, 2022

Codecov Report

Merging #2578 (062eed3) into main (7d21975) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2578      +/-   ##
==========================================
+ Coverage   79.50%   79.52%   +0.01%     
==========================================
  Files          49       49              
  Lines        5206     5211       +5     
==========================================
+ Hits         4139     4144       +5     
  Misses       1067     1067              
Impacted Files Coverage Δ
flax/linen/linear.py 97.55% <100.00%> (+0.04%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@@ -688,12 +695,17 @@ def __call__(self, inputs: Array) -> Array:
-(y_dim - x_dim) % (2 * x_dim)
for y_dim, x_dim in zip(y.shape[1:-1], scaled_x_dims)
]
# Divide the padding equaly between left and right. The choice to put
# Divide the padding equally between left and right. The choice to put
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could move this comment to the else branch and have a similar one for the positive branch?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, the comments there could be clearer. How about this modified version, with clearer comments for both cases?

(size_diff // 2, (size_diff + 1) // 2) for size_diff in size_diffs
]
else:
# This was the convention chosen previously and is kept here for
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for improving this! I have an additional comment, since else this is the default case can we keep the original explanation here? On the positive branch, can you explain why the +1 is now on the right? (e.g. "since the kernel is being transposed we add +1 on the right side instead to mirror the regular version".

Copy link
Collaborator

@cgarciae cgarciae left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mathisgerdes for doing this! Exposing transpose_kernel is a great idea.

@marcvanzee
Copy link
Collaborator

@mathisgerdes @cgarciae this PR has build failures so I have removed the "pull ready" label. Could you please fix those first?

@mathisgerdes
Copy link
Contributor Author

@mathisgerdes @cgarciae this PR has build failures so I have removed the "pull ready" label. Could you please fix those first?

I believe those were problems with the previous commit, not any changes here. I've rebased to the latest commit.

@cgarciae cgarciae added the Priority: P1 - soon Response within 5 business days. Resolution within 30 days. (Assignee required) label Dec 13, 2022
@mathisgerdes
Copy link
Contributor Author

Is there still something to be done here, @cgarciae, @andsteing?

Copy link
Collaborator

@cgarciae cgarciae left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay. LGTM.
Thanks for doing this!

@cgarciae cgarciae added pull ready and removed Priority: P1 - soon Response within 5 business days. Resolution within 30 days. (Assignee required) labels Jan 24, 2023
@copybara-service copybara-service bot merged commit 6d85aac into google:main Jan 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants