Computation of penalty terms

[rgemulla]

Commit e667cf9 changes the way penalties are interpreted for many models. The penalty term is currently computed only once per embedding, but with this change it's computed twice if subject and object embedder are the same (a common case). Instead of calling penalty twice, the code should check whether they are the same and, if so, call penalty only once.

[samuelbroscheit]

What is the reasoning? They are used twice hence the penalty is computed for each factor matrix of each mode, which does matter in two cases: 1. when the penalty that is computed is weighted 2. when the penelty weight comes from a shared lookup_embedder config setting, such that we cannot learn/tune the scaling.

[rgemulla]

I can see arguments for both ways of doing it.

1. Keep as before: Do it like everywhere else (all related work). More intuitive/natural. Better performance.

2. Change is in this commit: No special treatment of the shared_embedders case. Anything else?

The weights are not a big deal: can be done both ways: in (1) by simply passing along a list of ids (as suggested in #39), in (2) by calling penalty twice (for s, for o). (BTW: currently, the penalty function in KgeModel does not correctly pass along ids.)

[samuelbroscheit]

currently, the penalty function in KgeModel does not correctly pass along ids.

How is that?

[rgemulla]

currently, the penalty function in KgeModel does not correctly pass along ids.

How is that?

All fine, I must have been looking at outdated code.

[rgemulla]

The current implementation of these penalty terms for lookup_embedders uses embed:

parameters = self.embed(kwargs['batch']['triples'][:, kwargs['slot']])

This may be flawed since this will run dropout (but shouldn't).

[samuelbroscheit]

Argh, of course. Fixed.

…

On Wed, Aug 21, 2019 at 12:20 PM rgemulla ***@***.***> wrote: The current implementation of these penalty terms for lookup_embedders uses embed: parameters = self.embed(kwargs['batch']['triples'][:, kwargs['slot']]) This may be flawed since this will run dropout (but shouldn't). — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#41?email_source=notifications&email_token=AFMYSKY5XZESD5D7VFI4LITQFUJIJA5CNFSM4IM2LOA2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4ZGD6Q#issuecomment-523395578>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFMYSK3D3ZREMTWYD5C2VBDQFUJIJANCNFSM4IM2LOAQ> .

[rgemulla]

Looks fine now. The only suggested change I still have is the API change from #39:

I suggest to change self.get_s_embedder().penalty(slot=0, **kwargs) to self.get_s_embedder().penalty(penality_ids=triples[slot], **kwargs) or so. This way, the embedder can be used later on when the weights do not come from triples.

[rgemulla]

The change above is still open.

I also think that it would be helpful (for understanding the model) if the penalty terms were named (e.g., entity_embedder.l2) and traced/printed both with and without the regularization weight being applied.

[samuelbroscheit]

Is this currently being worked on already?

[rgemulla]

Yes, by @Nzteb

[rgemulla]

Closed with #101