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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃Ж 馃懛 Add interaction modules #242

Merged
merged 12 commits into from
Jan 22, 2021
Merged

馃Ж 馃懛 Add interaction modules #242

merged 12 commits into from
Jan 22, 2021

Conversation

cthoyt
Copy link
Member

@cthoyt cthoyt commented Jan 21, 2021

Closes #240

This PR adds the PyTorch module wrappers around each of the functional forms of the interaction functions proposed in the previous PR (#238) as well as tests, requisite utility functions, and documentation.

This PR doesn't bring over the literal modules, that's pushed until #244

src/pykeen/nn/modules.py Outdated Show resolved Hide resolved
@cthoyt cthoyt changed the title Add interaction modules 馃Ж 馃懛 Add interaction modules Jan 21, 2021
@cthoyt cthoyt requested a review from mberr January 22, 2021 02:56
cthoyt and others added 7 commits January 22, 2021 17:34
@cthoyt cthoyt marked this pull request as ready for review January 22, 2021 17:07
src/pykeen/nn/modules.py Outdated Show resolved Hide resolved
@cthoyt cthoyt merged commit c96e9ce into master Jan 22, 2021
@cthoyt cthoyt deleted the add-interaction-modules branch January 22, 2021 17:44


def _get_batches(z, slice_size):
for batch in zip(*(hh.split(slice_size, dim=1) for hh in ensure_tuple(z)[0])):
Copy link

Choose a reason for hiding this comment

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

Hi, I'm trying to learn the code. Why dim=1 is fixed here? I've thought dim should be passed from _forward_slicing_wrapper for different h, r, t.

Copy link
Member

Choose a reason for hiding this comment

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

Hi @Yevgnen ,

thanks for looking in such old PRs. This seems to be an error indeed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @Yevgnen, we just patched this in #683

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.

Extract functional modules
3 participants