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

SimplE model definition not correct? #151

Closed
dschaehi opened this issue May 8, 2019 · 5 comments
Closed

SimplE model definition not correct? #151

dschaehi opened this issue May 8, 2019 · 5 comments
Labels

Comments

@dschaehi
Copy link
Contributor

dschaehi commented May 8, 2019

Hi, as far as I know, SimplE defines for each entity two different embeddings: one for head and one for tail. So for a triple (i, j, k) you need to average h_i * r_j * t_k and h_j * r^{-1}_j * t_i. It seem that this is not reflected in your code. The model in your code seems to be symmetric, where as SimplE is not symmetric.

OpenKE/models/SimplE.py

Lines 23 to 24 in 2380e9d

def _calc_avg(self, h, t, r, r_inv):
return (- torch.sum(h * r * t, -1) - torch.sum(h * r_inv * t, -1))/2

@Joker-Song
Copy link
Contributor

Hi, as far as I know, SimplE defines for each entity two different embeddings: one for head and one for tail. So for a triple (i, j, k) you need to average h_i * r_j * t_k and h_j * r^{-1}_j * t_i. It seem that this is not reflected in your code. The model in your code seems to be symmetric, where as SimplE is not symmetric.

OpenKE/models/SimplE.py

Lines 23 to 24 in 2380e9d

def _calc_avg(self, h, t, r, r_inv):
return (- torch.sum(h * r * t, -1) - torch.sum(h * r_inv * t, -1))/2

the implementation reference https://github.com/Mehran-k/SimplE/blob/master/simplE_avg.py. In simpleE_avg.py h_i = t_j , t_k = h_j

@dschaehi
Copy link
Contributor Author

As you can see in the following line of original source code there are h1_emb and h2_emb for the head entities and t1_emb and t2_emb for tail entities. So SimplE has head and tail representations for each entity.

self.init_scores = (tf.reduce_sum(tf.multiply(tf.multiply(self.h1_emb, self.r1_emb), self.t1_emb), 1) + tf.reduce_sum(tf.multiply(tf.multiply(self.h2_emb, self.r2_emb), self.t2_emb), 1)) / 2.0

https://github.com/Mehran-k/SimplE/blob/7c9edc7d2c299fffd2164e976e312aa8996a0e28/simplE_avg.py#L33

@dschaehi dschaehi closed this as completed Oct 3, 2019
@dfdazac
Copy link

dfdazac commented Apr 7, 2020

@dschaehi was there a reason for closing? I think this issue is still present in the OpenKE implementation. In the original implementation the embedding for an entity depends on the position in the triple:

https://github.com/Mehran-k/SimplE/blob/29108230b63920afa38067b1aff8b8d53d07ed01/simplE_avg.py#L21-L35

@dschaehi
Copy link
Contributor Author

dschaehi commented Apr 7, 2020

@dfdazac: No, I closed it because there didn't seemed to be a response. I can reopen it.

@dschaehi dschaehi reopened this Apr 7, 2020
@Mehran-k
Copy link

Hi all, I'm the main author of the SimplE paper. I have received emails asking me if the OpenKE implementation of SimplE is correct or not so I thought I post a public response here. I can confirm that the OpenKE implementation is indeed incorrect and there are two issues (one major, one minor) in it:
Major issue: As pointed out by @dschaehi there's a major issue in the model definition. SimplE requires two embedding vectors per entity, one to be used when the entity is the head and one to be used when the entity is the tail. In the OpenKE implementation, there is only one embedding vector per entity which hurts the model by making it almost identical to DistMult.
Minor issue: This implementation corresponds to a variant of SimplE which we called SimplE-ignr in the paper. It takes the average of the two predictions during training but only uses one of the predictions during testing (see https://github.com/thunlp/OpenKE/blob/OpenKE-PyTorch/openke/module/model/SimplE.py#L54). The standard SimplE model takes the average of the two predictions for both training and testing.

For a correct pytorch implementation of SimplE, I recommend this repo: https://github.com/baharefatemi/SimplE/blob/master/SimplE.py

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants