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

decomposition of model_interfaces.py #272

Merged
merged 20 commits into from
Aug 16, 2021

Conversation

hstojic
Copy link
Collaborator

@hstojic hstojic commented Jun 22, 2021

Hello,

here is a draft for decomposing the model_interfaces (now also renamed to interfaces)

some remarks:

  • I didn't move optimizers yet, that would be the next step - it needs some further rethinking and changes in some classes as default gpflow optimizer is spreaded elsewhere
  • I haven't touched tests, after agreeing on final form I can do that part as well
  • it would be great to eliminate gpflow imports anywhere else from the code, isolate it to models_gpflow
  • this PR will make the toolbox incompatible with previous versions, so perhaps this is something to implement for trieste 1.x

@henrymoss henrymoss self-requested a review June 23, 2021 06:39
@hstojic hstojic marked this pull request as ready for review August 5, 2021 20:34
@hstojic
Copy link
Collaborator Author

hstojic commented Aug 5, 2021

Finally updated all this. Merged in latest develop, moved around all the tests, eliminated gpflow from other parts of the code.

I also attempted to fix config.py by creaating a parent class and creating model type specific subclass, but there typing turned out to be messy and mypy complains - I would appreciate some help in this department :) (@joelberkeley?)

I haven't touched optimizers yet, not crucial for now as everything can work without that part, so I left it for another PR.

Copy link
Collaborator

@uri-granta uri-granta left a comment

Choose a reason for hiding this comment

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

This looks great. I'll take a look at the typing next.

Note that it'll result in a small merge conflict for #320 (cc @sebastianober)

Also probably worth building the docs manually using tox to see that they still look ok.

@uri-granta uri-granta self-requested a review August 13, 2021 09:22
@uri-granta
Copy link
Collaborator

Looking in more detail at the typing failures, there are quite a few issues. Happy to discuss.

@hstojic
Copy link
Collaborator Author

hstojic commented Aug 16, 2021

All fixed now and merged in latest develop.

I returned get_kernel method to ProbabilisticModel and we'll address that in another PR.

@hstojic hstojic merged commit e301cf1 into develop Aug 16, 2021
@hstojic hstojic deleted the hrvoje/model_interfaces_decomposition branch August 16, 2021 19:18
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