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

temptative fix for the case where a calculator does not return anything #308

Closed liam-o-marsh closed 4 months ago

liam-o-marsh commented 4 months ago

simple python test:

import pyscf, rascaline
mol = rascaline.systems.wrap_system(pyscf.M(atom='H 0 0 -10; H 0 0 10'))
rascaline.calculators.NeighborList(5.0,False).compute(mol)

Since the cutoff is smaller than the distance between hydrogens, there are no pairs found by compute_neighbors, causing assert_ne!(keys.count(), 0); to panic (at rascaline/src/calculator.rs:59:9).

Those three lines prevent the panic by making Calculator::prepare() return an empty TensorMap early.


📚 Documentation preview 📚: https://rascaline--308.org.readthedocs.build/en/308/

Luthaf commented 4 months ago

Thanks you for the PR! This should be added as a test somewhere, either in Python or directly in the Rust code.

I'm not sure if an early return is the best solution here, since it might interact weirdly with key selections or samples/properties selection when giving predefined values. It think that returning an empty vec instead of the failing assert if there are no keys would be a nicer solution. I'll think of a test for this (slightly convoluted) case

liam-o-marsh commented 4 months ago

https://github.com/Luthaf/rascaline/actions/runs/9319582029/job/25654465095?pr=308 the fact that it failed like this on only one platform tastes like cosmic rays? anywho, yeah that way makes more sense

Luthaf commented 4 months ago

The CI error does seems unrelated, not sure why it is failing though. @PicoCentauri did you seen anything like this while writing the spliner code?

Luthaf commented 4 months ago

Ok, CI error should be fixed now. Can you rebase this branch on top of master?

liam-o-marsh commented 4 months ago

you're welcome!