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

add GPRegression by SSM demo #209

Merged
merged 6 commits into from
Sep 22, 2022
Merged

add GPRegression by SSM demo #209

merged 6 commits into from
Sep 22, 2022

Conversation

HoangMHNguyen
Copy link
Contributor

add 2 notebooks on implementing GP regression by state-space model

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@bvdmitri
Copy link
Member

@HoangMHNguyen , thanks for this demo! They are very quite good imo, but there are couple of points I would like you to address:

  • Your branch was outdated with the current master, I merged master into your branch, don't forget to do git pull first when you make new changes
  • Could you please merge two notebook into one? I would advise you to simply add Matern 5/2 example right after the Matern 3/2 as a new section in a single Jupyter notebook. Write something like: "Lets try to use a different kernel now..."
  • You made an impressive work explaining the model specification and made useful references, however the demo does not have any result analysis. To me it seems a bit off, such a nice introduction and a simple output at the end. Could you please add a bit of words at the end as well. Not much, but smth like:
    • "We need missing rules because..."
    • "Here how we can plot out results..."
    • "The results make sense, because ..."

Overall looks very good, please address these minor comments and we will merge 👍

@bvdmitri
Copy link
Member

Documentation is failing due to the PyPlot library, which is unrelated to this PR.

@HoangMHNguyen
Copy link
Contributor Author

@bvdmitri Thanks for your comments! I have created a new notebook that combines old ones and also added some words to explain the result and other stuff.

@codecov-commenter
Copy link

codecov-commenter commented Sep 22, 2022

Codecov Report

Base: 76.83% // Head: 76.83% // No change to project coverage 👍

Coverage data is based on head (f63174d) compared to base (23a7c95).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #209   +/-   ##
=======================================
  Coverage   76.83%   76.83%           
=======================================
  Files         203      203           
  Lines        6709     6709           
=======================================
  Hits         5155     5155           
  Misses       1554     1554           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@ismailsenoz ismailsenoz left a comment

Choose a reason for hiding this comment

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

I think it looks fine—well done Hoang.

@bvdmitri bvdmitri merged commit 6c22cb9 into master Sep 22, 2022
@bvdmitri bvdmitri deleted the develop-gp2 branch September 22, 2022 14:03
@albertpod albertpod added this to the Gaussian Processes milestone Oct 7, 2022
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.

5 participants