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

DotLayer could infer var1/var2 automatically #738

Closed
Zettelkasten opened this issue Nov 8, 2021 · 1 comment · Fixed by #856
Closed

DotLayer could infer var1/var2 automatically #738

Zettelkasten opened this issue Nov 8, 2021 · 1 comment · Fixed by #856

Comments

@Zettelkasten
Copy link
Member

This is more of a proposal for now. The idea is as follows:

For e.g. GatherLayer we automatically infer the shared axes between the positions and the params automatically using the dim tags. For me, this logic worked very well in the past.
In a same way, we could also infer the var1/var2 axes as be the axes which are not present in both inputs for DotLayer.
This would also work well for when axis change depending on automatic rec layer optimization, and make a common argument that could be specified instead of var1/var2 like discussed in #569 redundant.

I'm not 100% sure if this might be unexpected behavior though. E.g., to compute an dyadic product of a [F]-shaped tensor (i.e. compute v * v^T), the user could previously use DotLayer and set var1=var2=F as well as red1=red2=(), and then it would do exactly that, giving a [F1,F2] tensor.
Now with that change, you would first need to create a copy of the input, rename the axis to something different, and then use DotLayer.
But the old solution anyway had some issues: It would not be clear what the output dim tags (here F1 and F2) would be. They should would be the same (I'm really not sure what our implementation currently does), but then you could not reference the axes uniquely. That would be pretty bad.
With the new solution, we are very explicit what the output dim tags are.

Similarly, we can also get away with just a single reduce argument (#636).

Originally posted by @Zettelkasten in #627 (comment)

@albertz
Copy link
Member

albertz commented Nov 8, 2021

Wow, we were almost simultaneously. :) I will leave this open and close mine (#737).

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 a pull request may close this issue.

2 participants