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 helper functions to calculate required M for hsgp #7058

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

ferrine
Copy link
Member

@ferrine ferrine commented Dec 9, 2023

It is always a trouble to calculate required M for HSGP, adding helper functions to simplify that

What is this PR about?
...

Checklist

Major / Breaking Changes

  • ...

New features

  • Calculating HSGP m parameter for common kernels

Bugfixes

  • ...

Documentation

  • ...

Maintenance

  • ...

📚 Documentation preview 📚: https://pymc--7058.org.readthedocs.build/en/7058/

@ferrine ferrine requested a review from bwengals December 9, 2023 16:54
Copy link

codecov bot commented Dec 9, 2023

Codecov Report

Merging #7058 (9f18c43) into main (005ba5f) will decrease coverage by 0.70%.
Report is 2 commits behind head on main.
The diff coverage is 92.30%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #7058      +/-   ##
==========================================
- Coverage   92.16%   91.47%   -0.70%     
==========================================
  Files         101      101              
  Lines       16827    16840      +13     
==========================================
- Hits        15509    15404     -105     
- Misses       1318     1436     +118     
Files Coverage Δ
pymc/gp/cov.py 97.84% <92.30%> (-0.16%) ⬇️

... and 3 files with indirect coverage changes

int
Number of eigenvectors
"""
return ceil(1.75 * L / ls)
Copy link
Member

Choose a reason for hiding this comment

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

Can you link the source of these numbers in the docstring?

Copy link
Member

Choose a reason for hiding this comment

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

Also, you can rebase on main to fix these ❌ tests. I merged Ricardos #7057

@michaelosthege michaelosthege added enhancements GP Gaussian Process labels Dec 9, 2023
@@ -663,6 +690,28 @@ def power_spectral_density(self, omega: TensorLike) -> TensorVariable:
pow = pt.power(5.0 + pt.dot(pt.square(omega), pt.square(ls)), -1 * D52)
return (num / den) * pt.prod(ls) * pow

@staticmethod
def required_num_eigenvectors(L: float, ls: float) -> int:
r"""Number of eigenvectors in Hilbert space that approximate the GP well.
Copy link
Contributor

@bwengals bwengals Dec 10, 2023

Choose a reason for hiding this comment

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

Probably should add some more detail in the docstrings here? Would be kind of mysterious to see this if you're not using HSGPs, or know how to choose L.

Another idea (take it or leave it) would be to have this put into a single function in hsgp_approx.py that does this all in one go, something like

def required_num_eigenvectors(cov_func, L, ls):
    if cov_func.__class__.__name__ == "ExpQuad":
        return ceil(1.75 * L / ls)
    elif ...:
        ...

which maybe is better because the docstring can be in one place and the logic and purpose is isolated in one spot instead of spread out. I wonder if the recommendations may evolve too, since in the paper they didn't try for higher than 1D inputs.

Copy link
Member

Choose a reason for hiding this comment

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

Even more convenient would be to have this function called automatically if m is not passed to HSGP (along with a healthy warning message informing users what is going on).

Copy link
Contributor

Choose a reason for hiding this comment

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

and c too (but maybe not in this PR)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I'll do this

@@ -595,6 +600,28 @@ def power_spectral_density(self, omega: TensorLike) -> TensorVariable:
exp = pt.exp(-0.5 * pt.dot(pt.square(omega), pt.square(ls)))
return c * pt.prod(ls) * exp

@staticmethod
def required_num_eigenvectors(L: float, ls: float) -> int:
r"""Number of eigenvectors in Hilbert space that approximate the GP well.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add the paper (and maybe an equation reference) to understand where to learn more about the factors (for us, the newbies?)

@ricardoV94 ricardoV94 changed the title add helper functions to calculate required M for hsgp Add helper functions to calculate required M for hsgp Feb 6, 2024
@ricardoV94 ricardoV94 marked this pull request as draft February 6, 2024 10:49
@ricardoV94
Copy link
Member

Marked as a draft given all requests for changes in the review. Would be great to get this one over the finish line!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancements GP Gaussian Process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants