Skip to content

Gather: broadcast + expand - #83

Open
justinchiu wants to merge 2 commits into
harvardnlp:masterfrom
justinchiu:gatherbroadcast
Open

Gather: broadcast + expand#83
justinchiu wants to merge 2 commits into
harvardnlp:masterfrom
justinchiu:gatherbroadcast

Conversation

@justinchiu

@justinchiu justinchiu commented Feb 25, 2019

Copy link
Copy Markdown
Contributor

Adds broadcasting support for gather by adding dimensions (unsqueezing through _force_order using an overlapping order) and expanding.

Comment thread namedtensor/test_core.py
# Gather will move "b" and "c" to the front for t and index respectively
# so we must force the order in order to compare to the original
# torch.gather.
ntensor = ntorch.gather(t, "b", index, "c")._force_order(("a", "c"))

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.

This isn't a good unit test. It shouldn't call any _ functions.

@justinchiu justinchiu Feb 25, 2019

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.

High level question: Since we don't assume any ordering, is the right approach to try all permutations of the output ntensor and pass if any of them succeed (equal base)? Or should wer try to keep the underlying order the same as torch.* (although this may be unclear for ntorch.gather since broadcasting isn't defined in torch.gather).

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.

I think the ntorch.equal function will now do this automatically. But either way isn't the function you want just .transpose? More importantly does this test prove to me that your change works?

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.

Great, thanks. I agree that the unit test doesn't test anything, but I wanted to ask about how to compare first. I can write a better test.

@srush

srush commented Mar 1, 2019

Copy link
Copy Markdown
Contributor

Any updates?

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.

2 participants