librascal icon indicating copy to clipboard operation
librascal copied to clipboard

Feat/rkhs

Open ceriottm opened this issue 4 years ago • 2 comments

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)

ceriottm avatar May 25 '21 12:05 ceriottm

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?

max-veit avatar May 27 '21 15:05 max-veit

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.

max-veit avatar May 27 '21 16:05 max-veit