Skip to content

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.

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.

5 participants