Skip to content

Flax Regnet#21867

Merged
sgugger merged 6 commits into
huggingface:mainfrom
Shubhamai:flax-regnet
Apr 4, 2023
Merged

Flax Regnet#21867
sgugger merged 6 commits into
huggingface:mainfrom
Shubhamai:flax-regnet

Conversation

@Shubhamai

Copy link
Copy Markdown
Contributor

What does this PR do?

Flax Implementation of facebook/regnet-y-040

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

  • Flax: sanchit-gandhi

@HuggingFaceDocBuilderDev

HuggingFaceDocBuilderDev commented Mar 1, 2023

Copy link
Copy Markdown

The documentation is not available anymore as the PR was closed or merged.

Comment thread src/transformers/models/regnet/modeling_flax_regnet.py Outdated
@Shubhamai

Copy link
Copy Markdown
Contributor Author

@sanchit-gandhi This is now ready for your review, thanks a lot for your time.

Comment thread src/transformers/models/regnet/modeling_flax_regnet.py
@Shubhamai Shubhamai mentioned this pull request Mar 9, 2023
8 tasks

@sanchit-gandhi sanchit-gandhi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Very nice PR @Shubhamai! Just some small code clean-up comments - the JAX/Flax logic looks sound! In general, we can try and use # Copied from statements in every place we copy code from Flax ResNet one-for-one.

Comment thread docs/source/es/index.mdx Outdated
Comment thread docs/source/it/index.mdx Outdated
Comment thread docs/source/pt/index.mdx Outdated
Comment thread src/transformers/models/regnet/modeling_flax_regnet.py
Comment thread src/transformers/models/regnet/modeling_flax_regnet.py Outdated

pooled_output = outputs.pooler_output if return_dict else outputs[1]

logits = self.classifier(pooled_output[:, :, 0, 0])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Any reason we slice 0, 0 over the last two dims? Don't see this in the PyTorch code:

logits = self.classifier(pooled_output)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No specific reason, since we are using # Copied from ... from resnet, this needs to be kept same.

Comment thread src/transformers/models/regnet/modeling_flax_regnet.py
Comment thread src/transformers/modeling_flax_outputs.py
Comment thread tests/models/regnet/test_modeling_flax_regnet.py
Comment thread tests/models/regnet/test_modeling_flax_regnet.py Outdated
@sanchit-gandhi sanchit-gandhi mentioned this pull request Mar 24, 2023
7 tasks
@Shubhamai

Copy link
Copy Markdown
Contributor Author

@sanchit-gandhi All the requested changes have been made and looks ready for next iteration of review, thanks a lot for your time.

@sanchit-gandhi sanchit-gandhi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Very nice @Shubhamai - thanks for incorporating all of the comments from the Flax ResNet PR and the previous Flax RegNet review!

Looks ready for a final review!

)


# Copied from transformers.models.resnet.modeling_flax_resnet.FlaxResNetPreTrainedModel with ResNet->RegNet,resnet->regnet,RESNET->REGNET

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice!

@sanchit-gandhi sanchit-gandhi requested a review from sgugger April 4, 2023 15:39

@sgugger sgugger left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for adding this new model! Could you quickly rebase your branch on main? it just fix the issues with the CI.

@sgugger sgugger merged commit 9006774 into huggingface:main Apr 4, 2023
@Shubhamai Shubhamai deleted the flax-regnet branch April 4, 2023 16:42
raghavanone pushed a commit to raghavanone/transformers that referenced this pull request Apr 5, 2023
* initial commit

* review changes

* post model PR merge

* updating doc
novice03 pushed a commit to novice03/transformers that referenced this pull request Jun 23, 2023
* initial commit

* review changes

* post model PR merge

* updating doc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants