Luthaf / rascaline

Computing representations for atomistic machine learning
https://luthaf.fr/rascaline/
BSD 3-Clause "New" or "Revised" License
44 stars 13 forks source link

Add self pairs to NeighborList calculator #175

Closed rosecers closed 1 year ago

rosecers commented 1 year ago

I've added the option to return the self terms for a general neighborlist, as can be helpful from time to time. Coding done on the advice of Guillaume, so please no pitchforks on my first foray into Rust.


:books: Documentation preview :books:: https://rascaline--175.org.readthedocs.build/en/175/

github-actions[bot] commented 1 year ago

Here is a pre-built version of the code in this pull request: wheels.zip, you can install it locally by unzipping wheels.zip and using pip to install the file matching your system

rosecers commented 1 year ago

Looks good! Could you add a new test at the bottom of the file with self_pairs: true?

As soon as I figure out how, absolutely!

rosecers commented 1 year ago

I would like the calculator not to crash if we only have the self-term (and no other neighbours). Is this possible?

On 11 Apr 2023, at 03:59, Guillaume Fraux @.***> wrote:

@Luthaf commented on this pull request.

Looks good overall, I have one final question

In rascaline/src/calculators/neighbor_list.rs https://github.com/Luthaf/rascaline/pull/175#discussion_r1162508188:

@@ -58,12 +60,12 @@ impl CalculatorBase for NeighborList { }

 fn keys(&self, systems: &mut [Box<dyn System>]) -> Result<Labels, Error> {
  • assert!(self.cutoff > 0.0 && self.cutoff.is_finite());
  • assert!(self.cutoff >= 0.0 && self.cutoff.is_finite()); Why this change? If we really need a cutoff of 0, we should treat it separately, since most implementation assume a strictly positive cutoff.

— Reply to this email directly, view it on GitHub https://github.com/Luthaf/rascaline/pull/175#pullrequestreview-1378763087, or unsubscribe https://github.com/notifications/unsubscribe-auth/ALKVP3QYXEWVIAKRFLPJSO3XAUMQPANCNFSM6AAAAAAWVX46TU. You are receiving this because you authored the thread.

Luthaf commented 1 year ago

My first question is why do you need a calculator to get the list of atoms in your structure? Since that's basically the output with only self pairs.

Otherwise, I would try with a cutoff of 1e-3 instead of 0, which should be small enough to only include self pairs.