transformers icon indicating copy to clipboard operation
transformers copied to clipboard

Added type hints for `Graphormer` pytorch version

Open dewasahu2003 opened this issue 1 year ago • 1 comments

@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

dewasahu2003 avatar Apr 30 '23 11:04 dewasahu2003

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

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

Rocketknight1 avatar May 02 '23 12:05 Rocketknight1

@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🙂

dewasahu2003 avatar May 02 '23 12:05 dewasahu2003

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.

Rocketknight1 avatar May 03 '23 12:05 Rocketknight1

@Rocketknight1 Hi 👋

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

dewasahu2003 avatar May 03 '23 12:05 dewasahu2003

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

Rocketknight1 avatar May 03 '23 13:05 Rocketknight1

Sure

dewasahu2003 avatar May 15 '23 15:05 dewasahu2003

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

Rocketknight1 avatar May 15 '23 17:05 Rocketknight1