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

Feat/rkhs #354

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Feat/rkhs #354

wants to merge 15 commits into from

Conversation

ceriottm
Copy link
Contributor

Implement an RKHS solver for the sparse GPR problem.

Rationale and detailed description of the changes:

Solving the GPR problem using the "normal" equation is very ill-conditioned.
This implements an alternative solver that is much better behaved.
Might be good to also include different options in terms of using solve or lstsq,
let's see where this goes - however this already works.

Tasks before review:

  • Add tests, examples, and complete documentation of any added functionality
    • Make sure the autogenerated documentation appears in Sphinx and is
      formatted correctly (ask @max-veit if you need help with this task).
    • Write additional documentation in the Sphinx (not docstrings!) to
      explain the feature and its usage in plain English
    • Make sure the examples run (!) and produce the expected output
    • For bugfix pull requests, list here which tests have been added or re-enabled
      to verify the fix and catch future regressions as well as similar bugs
      elsewhere in the code.
  • Run make lint on the project, ensure it passes
    • Run make pretty-cpp and make pretty-python, check that the
      auto-formatting is sensible
    • Review variable names, make sure they're descriptive (ask a friend)
    • Review all TODOs, convert to issues wherever possible
    • Make sure draft, in-progress, and commented-out code is moved to its
      own branch
  • If committing any code in Jupyter/IPython notebooks, install and run nbstripout
  • Merge with master and resolve any conflicts
  • (anything else that's still in progress)

@max-veit
Copy link
Contributor

This is good; how about moving the solvers into their own function (taking just the matrices K_NM, K_MM, and Y), separate from the setup/manager/saving logic implemented in train_gap_model?

and computing P_NM = K_NM @ U_MM @ Lam_MM^(-1.2) and then solves the linear
problem for those (which is usually better conditioned)
(P_NM.T@P_NM + 1)^(-1) P_NM.T@Y
by default, "Normal"
Copy link
Contributor

Choose a reason for hiding this comment

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

Document the "QR" mode please - how is it different from "RKHS"?

@max-veit
Copy link
Contributor

Btw, merging with the main branch will now fix two out of the three test failures. The remaining failure is due to unit test coverage, which I suggest we do not ignore.

@@ -428,6 +429,15 @@ def train_gap_model(
jitter : double, optional
small jitter for the numerical stability of solving the linear system,
by default 1e-8
solver: string, optional
method used to solve the sparse KRR equations.
"Normal" uses a least-squares solver for the normal equations:
Copy link
Contributor

Choose a reason for hiding this comment

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

"Normal" feels strange as a name. How about Standard or Direct or Direct Least Square?

force restart CI
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.

4 participants