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

Added type hints for Graphormer pytorch version #23073

Merged

Conversation

dewasahu2003
Copy link
Contributor

@Rocketknight1 👋

  • I added type hint for graphormer pytorch
  • checked formatting with black and ruff

if some checks on ci/cd do not, please do comment and correct

added type hints for graphormers pytorch , checked formating issues .
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Apr 30, 2023

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

@Rocketknight1
Copy link
Member

This looks pretty good! Is there a reason to use Union[torch.Tensor, torch.LongTensor] instead of just torch.LongTensor?

@dewasahu2003
Copy link
Contributor Author

@Rocketknight1 Hi 👋

  • Union[torch.Tensor, torch.LongTensor] is used because the file had a lot of nn.embedding instances which expects either IntTensor or LongTensor
  • so to avoid any confusion 😕 i used that
  • nn.embedding docs

image

if still changes are required i would be happy to make it🙂

@Rocketknight1
Copy link
Member

Hi @dewasahu2003, I think in most cases we just annotate those types as LongTensor! Your version is probably more correct, but for simplicity just LongTensor is fine, since that's what people usually use.

@dewasahu2003
Copy link
Contributor Author

@Rocketknight1 Hi 👋

  • if LongTensor is preferred then i would make changes along
  • that would help the code to be 🤒 bloat free

@Rocketknight1
Copy link
Member

Yep, I think replacing with LongTensor is slightly better, and does make the code a bit cleaner too.

Copy link
Member

@Rocketknight1 Rocketknight1 left a comment

Choose a reason for hiding this comment

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

This looks good to me now! Are you ready for me to merge it?

@dewasahu2003
Copy link
Contributor Author

Sure

@Rocketknight1 Rocketknight1 merged commit c2393ca into huggingface:main May 15, 2023
@Rocketknight1
Copy link
Member

Done. Thanks for the PR, we really appreciate it!

sheonhan pushed a commit to sheonhan/transformers that referenced this pull request Jun 1, 2023
* Added type hints for `Graphormer` pytorch version

added type hints for graphormers pytorch , checked formating issues .

* made the code less bloated
gojiteji pushed a commit to gojiteji/transformers that referenced this pull request Jun 5, 2023
* Added type hints for `Graphormer` pytorch version

added type hints for graphormers pytorch , checked formating issues .

* made the code less bloated
novice03 pushed a commit to novice03/transformers that referenced this pull request Jun 23, 2023
* Added type hints for `Graphormer` pytorch version

added type hints for graphormers pytorch , checked formating issues .

* made the code less bloated
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.

3 participants