Penalizing reciprocal relations when reciprocal technique is used

[Kenkoko]

Hi Prof Gemulla, Daniel,

I am Dung Huynh, a master's thesis student in the Data Science program. During my thesis, I found a small mistake from LibKGE such that the framework does not penalize the reciprocal relations when using the reciprocal technique.

Therefore, encouraged by Daniel, I would like to open a pull request to fix this mistake.

Best regards, Dung Huynh

@rufex2001, @rgemulla

[rgemulla]

Thanks! Shouldn't this code be moved to ReciprocalRelationsModel#penalty?

[Kenkoko]

Hi Prof Gemulla,

I have moved the code to ReciprocalRelationsModel#penalty. Please let me know what you think about this.

[rgemulla]

Looks good!

But: you may need to replace int(self.dataset.num_relations() / 2) by self.dataset.num_relations(). Please double-check.

[Kenkoko]

Thank Prof Gemulla,

I have fixed that mistake. Please let me know what you think about the code now ^^.

[rgemulla]

Looks good to me, thanks! @rufex2001 Agreed?

[rufex2001]

Looks good! There is a typo, though. Replace "recriprocal" with "reciprocal" and it's ready to merge

[Kenkoko]

Hi Prof. @rgemulla, @rufex2001 I have fixed the typo; please feel free to give me your thought ^^

[rgemulla]

I think the implementation is not correct for unweighted regularization: here the reciprocal relations were penalized (see LookupEmbedder#_get_regularize_weight) in the unmodified implementation. The bug was only for weighted regularization. Right?

[Kenkoko]

That's true; the bug was only for weighted regularization.

The "normal" and reciprocal relations were all penalized when performing the penalization for unweighted regularization regularize_args.weighted == false. However, when performing weighted regularization regularize_args.weighted == true, the reciprocal relations were not penalized.

I think my condition is wrong now, instead of

if "batch" in kwargs and "triples" in kwargs["batch"]:

it should be

if self.get_option("regularize_args.weighted"):

Because we just need to penalize the reciprocal relations when performing weighted regularization.

Please let me know what you think

[rgemulla]

Yes. And only if self.regularize != "" and self.get_option("regularize_weight") != 0.0

[Kenkoko]

Hi Prof Gemulla,

I have updated the checking conditions. Please let me know what you think about that.

Personally, I think adding self.regularize != "" and self.get_option("regularize_weight") != 0.0 makes code redundant because in lookup_embedder#penalty, we also perform that checking.

[rgemulla]

Looks good to me, thanks! It's also not redundant because it avoids unnecassary computations (such as increasing the relation indexes).

@rufex2001 Agreed as well? If so, please squash & merge.

[Kenkoko]

Wow, I didn't think about that, thank you for letting me know this ^^

[rufex2001]

Looks good! @rgemulla nice catch on avoiding a double penalty application of inverse relations on the unweighted setting.

Merging now! @Kenkoko thanks for the find and fix!

[Kenkoko]

@rgemulla @rufex2001 thanks for letting me contribute to this project, it's my pleasure to work with you. ^^