Skip to content

Added support for arbitrary tensors for FRN. #1496

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

Closed
wants to merge 12 commits into from

Conversation

Opletts
Copy link

@Opletts Opletts commented Mar 30, 2020

I'll write the test cases, just wanted to confirm if my changes are correct.
@Squadrick @saurabhme
In case of FC Layers with dimensions of B x N, we normalize across N. What's the shape of self.gamma and self.beta supposed to be? I've used 1 x N for now, let me know if I've misunderstood something.

@boring-cyborg boring-cyborg bot added the layers label Mar 30, 2020
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@bot-of-gabrieldemarmiesse

@Smokrow

You are owner of some files modified in this pull request.
Would you kindly review the changes whenever you have the time to?
Thank you very much.

@Opletts
Copy link
Author

Opletts commented Mar 30, 2020

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

Copy link
Member

@Squadrick Squadrick left a comment

Choose a reason for hiding this comment

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

@Opletts Thanks for the PR, a few suggested changes.

Copy link
Member

@Squadrick Squadrick left a comment

Choose a reason for hiding this comment

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

@Opletts The changes look good, can you add the test cases?

@Opletts
Copy link
Author

Opletts commented Apr 3, 2020

@Squadrick Modified the test cases, limited the tensor sizes to 7 because my system ran out of memory, shouldn't be an issue though.

@Opletts Opletts requested a review from Squadrick April 3, 2020 14:13
@Opletts
Copy link
Author

Opletts commented Feb 15, 2021

@Squadrick @AakashKumarNain just went through the paper again and it doesn't make sense for 2D tensors. FRN is a global normalisation over the spatial extent and that it's done on a per channel basis.

  • For 4D tensors with shape = [B x H x W x C], it makes sense that we normalise over the spatial dimensions [H, W].
  • For 3D tensors, it HAS to be of form [B x H x C] or [B x C x H], otherwise FRN doesn't make sense.
  • 2D tensors have to be of form [H x C] and if it's a single sample, the user should reshape it to [1 x H x C] and then use FRN.

Is my understanding completely flawed? Was I doing the right thing earlier?

@Squadrick
Copy link
Member

@Opletts You're right. We assume that the 0th index is batch, so we need 3D at least.

* Tests for 3D-7D tensors
* Error raised when tensors aren't at least 3D
@Opletts
Copy link
Author

Opletts commented Feb 26, 2021

@Squadrick @AakashKumarNain let me know if everything looks alright, I'll make changes if required.

@seanpmorgan
Copy link
Member

Thank you for your contribution. We sincerely apologize for any delay in reviewing, but TensorFlow Addons is transitioning to a minimal maintenance and release mode. New features will not be added to this repository. For more information, please see our public messaging on this decision:
TensorFlow Addons Wind Down

Please consider sending feature requests / contributions to other repositories in the TF community with a similar charters to TFA:
Keras
Keras-CV
Keras-NLP

@seanpmorgan seanpmorgan closed this Mar 1, 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.

5 participants