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

MGP #60

Closed
wants to merge 46 commits into from
Closed

MGP #60

wants to merge 46 commits into from

Conversation

gpfins
Copy link
Contributor

@gpfins gpfins commented Aug 7, 2017

In this pull request I implemented the Approximatively Marginalised GP as described as indicated in issue #39 . It currently supports multi-output GPs. A notebook and some tests are included.

@icouckuy
Copy link
Contributor

icouckuy commented Aug 7, 2017

Awesome, thanks. I see that the Travis tests aren't triggered because of an external PR.

In init.py you removed the objective import by accident I think, as well as forgot to remove testmgp.py

But we will fix that once I moved the branch and created a new PR

@javdrher
Copy link
Member

javdrher commented Aug 7, 2017

This is not to be included in the 0.1 release, preferable after tf 1.3 actual release

I suggest to fix things first, then create an in-house branch as @nknudde has no rights to push after that. Some things to consider:

  • we keep MGP in a file models or a file mgp.py? I favor the current setup as it will allow us to do the same thing we applied to acquisition (subpackage) if the file gets big
  • This requires tf 1.3 as the Hessian of the Cholesky won't work prior to that. Should we bump the version of GPflowOpt, or just skip this test if executed on a lower version? As i guess we would test on travis on 1.3 it seems natural to actually recommend tf 1.3, mentioning that anything starting from 1.0 should work apart from the mgp but is no longer tested that way.

@codecov-io
Copy link

codecov-io commented Aug 7, 2017

Codecov Report

Merging #60 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #60      +/-   ##
==========================================
+ Coverage   99.78%   99.79%   +0.01%     
==========================================
  Files          16       17       +1     
  Lines         928      987      +59     
==========================================
+ Hits          926      985      +59     
  Misses          2        2
Impacted Files Coverage Δ
GPflowOpt/__init__.py 100% <100%> (ø) ⬆️
GPflowOpt/models.py 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a473070...b4c3615. Read the comment docs.

@javdrher javdrher self-requested a review August 7, 2017 21:54
return self.build_predict(fmean, fvar, theta)

@AutoFlow((float_type, [None, None]), (float_type, [None, None]))
def predict_density(self, Xnew, Ynew):
Copy link
Member

Choose a reason for hiding this comment

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

Still missing a test for predict density :)

@javdrher
Copy link
Member

javdrher commented Aug 7, 2017

Small general guideline: lets avoid rebases. For this PR I'm ok with it, and we won't start from scratch.

Rebases are actually very good but for normal circumstances I think they make the history confusing. its a very good tool for complex merge scenario with an enormous amount of conflicts expected as it allows you to resolve conflicts as they occur, commit by commit (think of a fork which has gone its own way for a while and then is merged). For normal scenarios a merge is preferred.

@@ -8,7 +8,7 @@ python:
cache: pip
install:
- pip install -U pip wheel
- pip install tensorflow==1.0.1
- pip install tensorflow==1.3.0rc0
Copy link
Contributor

Choose a reason for hiding this comment

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

can we later specify this without a version number so we just always test against the latest stable? Which is the goal anyways, or rather work with the latest stable GPflow which hopefully follows the latest stable TF closely.

@icouckuy
Copy link
Contributor

icouckuy commented Aug 8, 2017

I'm also against rebases (at least I am now, some bad experiences). I thought it might be necessary to fix the commit history a bit as i thought it was corrupted due to rebranching, but it actually looks okay ( though there are several commits with the same name)

@javdrher javdrher mentioned this pull request Aug 9, 2017
@javdrher javdrher closed this Aug 10, 2017
@javdrher javdrher mentioned this pull request Aug 10, 2017
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

4 participants