Seg fault in ring_statistics.cpp following recent changes to neighbour list about matscipy HOT 16 CLOSED

@pastewka I could reproduce the bug and I think I found the problem in first_neighbours. If the changes don't affect the triplet list and the bond order potential I will commit them soon.

from matscipy.

Thanks for the fast response!

from matscipy.

@chcltchunk Can you please fix this? Nevertheless, the ring statistics should not segfault if something stupid is passed as the argument. @chcltchunk Can you turn the current segfault into a test, i.e. simply pick whatever the current output is and paste this into a test (that will segfault). I will then debug this and turn the segfault into a proper error message.

from matscipy.

As it turned out, the segmentation fault was somehow related to the seed array being initialized with 0 instead of -1. A test that makes sense to me would check whether the resulting first neighbour list only contains reasonable numbers within the range of len(atoms) and if 0 occurs only once?

from matscipy.

from matscipy.

Thanks both!

from matscipy.

There are still tests failing. Sorry for this mess, I shouldn't have merged to pull requests. (Somehow github does not show failing Travis tests in the PR so I was under the impression that those were good.)

What I'll do today is rearrange the testing framework (I want it to work with pytest) and fix those failing tests in that process.

from matscipy.

Sounds good. I was also confused about the CI setup. Travis is still running tests on PR branches, but for some reason they don’t show up in the GitHub PR interface.

from matscipy.

from matscipy.

Still working fine for libAtoms/QUIP repo, so I think it’s a configuration issue

from matscipy.

I've fixed all lingering issues, @jameskermode can you confirm that the ring statistics segfault has disappeared?

from matscipy.

Yup, seg fault is fixed - thanks!

python setup.py test still seems to run via unittest rather than pytest, I'll see if I can fix it. I also have some failing tests in test_crack.py, probably due to numerical precision.

from matscipy.

python setup.py test still seems to run via unittest rather than pytest, I'll see if I can fix it.

Done in master directly

from matscipy.

Okay, thanks. It was unclear to me if all those tests actually ran before. I think only a subset may have run.

from matscipy.

Some of them depend on atomistica.

from matscipy.

Ahh, okay. Enabling those in CI is also something that I should look at.

from matscipy.