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 GPs #632

Merged
merged 70 commits into from
Mar 20, 2023
Merged

Add GPs #632

merged 70 commits into from
Mar 20, 2023

Conversation

tomicapretto
Copy link
Collaborator

@tomicapretto tomicapretto commented Jan 29, 2023

This is a work in progress and I expect this PR to be open for a considerable amount of time while I work on it.

Alright, I think this is done. We now have GPs through the HSGP approximation. For more information see the notebooks in docs/notebooks/hsgp_1d.ipynb and docs/notebooks/hsgp_2d.ipynb as well as the tests.

Before merging, we need pymc-devs/pymc#6458 to be merged in PyMC.
And before releasing, we need a new PyMC release with HSGP.

@bambinos bambinos deleted a comment from review-notebook-app bot Jan 29, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jan 29, 2023

Codecov Report

Merging #632 (4929d79) into main (787e1f1) will decrease coverage by 0.01%.
The diff coverage is n/a.

❗ Current head 4929d79 differs from pull request most recent head dbbd974. Consider uploading reports for the commit dbbd974 to get more accurate results

@@            Coverage Diff             @@
##             main     #632      +/-   ##
==========================================
- Coverage   86.69%   86.68%   -0.01%     
==========================================
  Files          37       37              
  Lines        2209     2208       -1     
==========================================
- Hits         1915     1914       -1     
  Misses        294      294              

see 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@tomicapretto tomicapretto marked this pull request as ready for review March 14, 2023 17:35
@@ -0,0 +1,799 @@
{
Copy link
Collaborator

@aloctavodia aloctavodia Mar 18, 2023

Choose a reason for hiding this comment

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

later \pmb{u} can be easily confused with \mu, maybe use a letter that stand out more clearly.


Reply via ReviewNB

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm still unsure about the change. I understand the possible confusion, but I don't want invent a new notation. \pmb{u} is usually used in mixed models literature and so I used it./ Other letter I can think of is \pmb{b}... I'll keep thinking about it

@@ -0,0 +1,799 @@
{
Copy link
Collaborator

@aloctavodia aloctavodia Mar 18, 2023

Choose a reason for hiding this comment

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

A link function, $g$, is...


Reply via ReviewNB

@@ -0,0 +1,799 @@
{
Copy link
Collaborator

@aloctavodia aloctavodia Mar 18, 2023

Choose a reason for hiding this comment

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

Going back to planet Earth...


Reply via ReviewNB

@@ -0,0 +1,799 @@
{
Copy link
Collaborator

@aloctavodia aloctavodia Mar 18, 2023

Choose a reason for hiding this comment

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

It used to be the case that az.style.use("arviz-darkgrid") (the same for any matplotlib style) has to be defined in a different cell than the one used to import matplotlib. Otherwise only part of the style was actually applied. Not sure if this is still the case


Reply via ReviewNB

Copy link
Collaborator Author

@tomicapretto tomicapretto Mar 20, 2023

Choose a reason for hiding this comment

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

I wasn't aware of this. Maybe related to this is that sometimes I had to use %matplotlib inline if I wanted styles to apply. Weird Jupyter magic.

But I think it's not the case anymore (at least here)

@@ -0,0 +1,799 @@
{
Copy link
Collaborator

@aloctavodia aloctavodia Mar 18, 2023

Choose a reason for hiding this comment

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

Formulae has its terms

Bambi has its terms


Reply via ReviewNB

@review-notebook-app
Copy link

review-notebook-app bot commented Mar 18, 2023

View / edit / reply to this conversation on ReviewNB

aloctavodia commented on 2023-03-18T20:01:06Z
----------------------------------------------------------------

This article demonstrates how to use Bambi

Can we link to Bill's video from PyMCON (I think is still not available...)


tomicapretto commented on 2023-03-20T19:19:25Z
----------------------------------------------------------------

It's still unavailable. I'll add it once we have it.

@review-notebook-app
Copy link

review-notebook-app bot commented Mar 18, 2023

View / edit / reply to this conversation on ReviewNB

aloctavodia commented on 2023-03-18T20:01:07Z
----------------------------------------------------------------

The default sampler is too slow? Can we use it? Just to simplify the notebook.


tomicapretto commented on 2023-03-20T19:21:41Z
----------------------------------------------------------------

Yeah, the default sampler is way slower. Not in all cases, but to give an example, it would take one or two minutes when "nuts_numpyro" took ten seconds. For iterative development, "nuts_numpyro" was way better.

@review-notebook-app
Copy link

review-notebook-app bot commented Mar 18, 2023

View / edit / reply to this conversation on ReviewNB

aloctavodia commented on 2023-03-18T20:01:08Z
----------------------------------------------------------------

The concept of "basis vectors" has not been introduced, given the emphasis in this section at least a couple of sentences explaining their role should be added.


Copy link
Collaborator Author

It's still unavailable. I'll add it once we have it.


View entire conversation on ReviewNB

Copy link
Collaborator Author

Yeah, the default sampler is way slower. Not in all cases, but to give an example, it would take one or two minutes when "nuts_numpyro" took ten seconds. For iterative development, "nuts_numpyro" was way better.


View entire conversation on ReviewNB

@tomicapretto tomicapretto merged commit 098be6f into bambinos:main Mar 20, 2023
@tomicapretto tomicapretto deleted the gps branch March 20, 2023 21:26
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.

3 participants