Skip to content
This repository was archived by the owner on Jan 15, 2026. It is now read-only.

Feat/sparsified soap#265

Merged
felixmusil merged 70 commits intomasterfrom
feat/sparsified_soap
May 22, 2020
Merged

Feat/sparsified soap#265
felixmusil merged 70 commits intomasterfrom
feat/sparsified_soap

Conversation

@felixmusil
Copy link
Copy Markdown
Contributor

@felixmusil felixmusil commented Mar 23, 2020

This PR allow for the sparsification of the powerspectrum.
Changes on the C++ side:

  • loops over (n1, n2, l) in the computation of powerspectrum are now over a compound index that contains all the information for indexing the powerspectrum and the spherical coefficients.
  • by default this compound index is initialized to replicate the former loops over (n1, n2, l)
  • if 'coefficient_subselection' with form {'a': [...], 'b': [...], 'n1': [...], 'n2': [...], 'l': [...]} is in the hypers then only the coefficients of the powerspectrum with species (a,b) and (n1,n2,l) will be computed. The resulting powerspectrum has one dummy key (0,0) which contains all the selected coefficients.
  • all of these 'features' are tested in test_calculator.cc using a slightly different take on the test organization. Tell me what you think please.

Changes on the python side:

  • the powerspectrum know how to create a mapping between dense linear index to the corresponding (a,b,n1,n2,l)
  • a FPS wrapper class that will be used to showcase the sparsification of the features in a notebook

@felixmusil felixmusil added the enhancement New feature or request label Mar 23, 2020
@felixmusil felixmusil added this to the ML model training milestone Mar 23, 2020
@felixmusil felixmusil self-assigned this Mar 23, 2020
@felixmusil felixmusil requested a review from max-veit May 13, 2020 15:08
Copy link
Copy Markdown
Contributor

@max-veit max-veit left a comment

Choose a reason for hiding this comment

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

Part the Second, in which the C++ code is reviewed.

Ok, I can follow what's going on for the most part, although some features need better documentation (see comments). It seems that the sparsification has the nice side effect of reducing the nesting depth in many loops, which definitely aids understanding. New test structure looks good and is much easier to understand.

@felixmusil felixmusil requested a review from max-veit May 19, 2020 08:32
@max-veit
Copy link
Copy Markdown
Contributor

Um, how about changing the Python formatter in a different branch and PR? There's enough changes mixed up into this one already.

@felixmusil
Copy link
Copy Markdown
Contributor Author

Sure that's how it should be done.

@max-veit
Copy link
Copy Markdown
Contributor

Then why does the latest commit add black.cmake? It's hidden in a commit with the undescriptive name comments, so it'll be hard to track where it got added from the log.

@felixmusil
Copy link
Copy Markdown
Contributor Author

right got added by mistake I guess.

Copy link
Copy Markdown
Contributor

@max-veit max-veit left a comment

Choose a reason for hiding this comment

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

now OK to merge by me

@felixmusil felixmusil merged commit 98d2933 into master May 22, 2020
@felixmusil felixmusil deleted the feat/sparsified_soap branch May 22, 2020 15:04
max-veit added a commit that referenced this pull request May 29, 2020
(Update to version of CURFilter and FPSFilter from PR #265)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants