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

♻️ 👌 Update interaction function resolution for new style models #387

Merged

Conversation

mberr
Copy link
Member

@mberr mberr commented Apr 20, 2021

follow-up of #384

@@ -58,7 +58,8 @@ def __init__(
# https://github.com/ibalazevic/multirelational-poincare/blob/34523a61ca7867591fd645bfb0c0807246c08660/model.py#L52
# uses float64
super().__init__(
interaction=MuREInteraction(p=p, power_norm=power_norm),
interaction=MuREInteraction,
interaction_kwargs=dict(p=p, power_norm=power_norm),
Copy link
Member Author

Choose a reason for hiding this comment

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

We could also directly put this as default value into the __init__ method's signature:

def __init__(
  self,
  ...,
  interaction_kwargs=frozendict(p=p, power_norm=power_norm),
  ...,
)

Copy link
Member

Choose a reason for hiding this comment

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

Not exactly sure if this is the same as 859db48. Why is frozendict necessary?

Copy link
Member

Choose a reason for hiding this comment

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

Nevermind the last comment. I don't think you can actually do what you are suggesting since we're passing p and power_norm in the init itself

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I was thinking about replacing it.

Before

 def __init__(
        ...,
        p: int = 2,
        power_norm: bool = True,
        ...,
    ) -> None:

After

 def __init__(
        ...,
        interaction_kwargs=frozendict(p=2, power_norm=True),
        ...,
    ) -> None:

The second option makes it directly clear where the parameter go to.

Copy link
Member

Choose a reason for hiding this comment

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

Disagree. The point of the high level model class is to abstract away knowing where these parameters go or how they're handled. It should be completely flat not only for users, but also since this is how the HPO and CLI builders work

@cthoyt
Copy link
Member

cthoyt commented Apr 20, 2021

@PyKEEN-bot test plz

@cthoyt cthoyt changed the title Update interaction function resolution for new style models ♻️ 👌 Update interaction function resolution for new style models Apr 20, 2021
@cthoyt cthoyt merged commit f734aaf into master Apr 20, 2021
@cthoyt cthoyt deleted the update-interaction-function-resolution-for-new-style-models branch April 20, 2021 11:52
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.

None yet

3 participants