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

Remove abc #41

Merged
merged 6 commits into from
Oct 19, 2021
Merged

Remove abc #41

merged 6 commits into from
Oct 19, 2021

Conversation

metodj
Copy link
Contributor

@metodj metodj commented Oct 12, 2021

  • removed all ABC stuff (abstract methods etc.)
  • refactored baselaplace.py -> BaseLaplace now has two subclasses, ParametricLaplace and FunctionalLaplace (this will greatly help with bringing in the GP inference)

@aleximmer aleximmer requested review from aleximmer and runame October 12, 2021 22:24
@aleximmer
Copy link
Owner

It looks good to me and since tests pass I don’t see a problem. However, I would suggest to make the raised errors a bit more verbose. For example, if an interface does not implement gradients, this could be explicitly stated as the cause of the problem with a message like “class XY does not provide gradients.” Or something similar. What do you think?

@metodj
Copy link
Contributor Author

metodj commented Oct 14, 2021

Sure, will make raised NotImplementedError more verbose. Another thing I just remembered is that the documentation will also have to be updated, will add this to the next commit as well. The change in the documentation will be minor, though (mostly renaming of the classes).

As an aside, the proposed refactor is backward-compatible in a sense that ParametricLaplace supports everything that BaseLaplace supported before. What is different now is that BaseLaplace now is more generic so that it will be easier to bring in FunctionalLaplace.

@metodj
Copy link
Contributor Author

metodj commented Oct 15, 2021

I created a dummy class in curvature.py in order to test how are the raised NotImplementedError displayed:

class TestInterface(CurvatureInterface):

    def __init__(self, model, likelihood, last_layer=False):
        super().__init__(model, likelihood, last_layer)

When using simply

    def gradients(self, x, y):
        raise NotImplementedError

the following is raised,

Screenshot 2021-10-15 at 13 27 24
while using

    def gradients(self, x, y):
        raise NotImplementedError(f"{self.__class__.__name__} does not provide gradients.")

yields
Screenshot 2021-10-15 at 13 28 00

If you prefer the second option, I can change all raise NotImplentedErrors accordingly. In my opinion, though, this might not be necessary as the error thrown is quite informative as it is and also by making it more verbose we would be slightly deviating from the Python convention.

@metodj
Copy link
Contributor Author

metodj commented Oct 15, 2021

Also, in the laplace.py I changed it so that only subclasses of ParametricLaplace are exposed through Laplace for the time being. Once FunctionalLaplace is ready, we can switch back to BaseLaplace. Let me know if that's fine.

@aleximmer
Copy link
Owner

Okay, we can leave the plain NotImplementedErrors then. That's actually verbose enough. I think you only need to recompile the documentation (see the readme for this) and then push the changed documents. Let me know if that doesn't work for you.
@runame Are you okay with merging this in?

Copy link
Collaborator

@runame runame left a comment

Choose a reason for hiding this comment

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

Sorry for the late reply, I was pretty busy this week. Looks good from my side, nice work!

One nitpick: the string style is inconsistent in some places, i.e. " " instead of our convention ' '. I'll comment on some lines where I have seen it (haven't checked everything carefully though).

laplace/baselaplace.py Outdated Show resolved Hide resolved
laplace/baselaplace.py Outdated Show resolved Hide resolved
laplace/baselaplace.py Outdated Show resolved Hide resolved
@metodj
Copy link
Contributor Author

metodj commented Oct 18, 2021

Thanks for pointing out the convention for the string style, wasn't aware of it before

@metodj metodj requested a review from runame October 18, 2021 06:54
Copy link
Collaborator

@runame runame left a comment

Choose a reason for hiding this comment

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

Nice, thanks!

@aleximmer aleximmer merged commit dd578dd into aleximmer:main Oct 19, 2021
@aleximmer
Copy link
Owner

Thanks both!

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.

4 participants