-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Conversation
Codecov Report
Additional details and impacted files@@ 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
|
int | ||
Number of eigenvectors | ||
""" | ||
return ceil(1.75 * L / ls) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
@@ -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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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?)
Marked as a draft given all requests for changes in the review. Would be great to get this one over the finish line! |
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
m
parameter for common kernelsBugfixes
Documentation
Maintenance
📚 Documentation preview 📚: https://pymc--7058.org.readthedocs.build/en/7058/