perf: features.contact

[DaniBodor]

Massively simplified code for addition of edge features in features.atomic_contact.
Also fixed mistake in LJ potential calculation
Removed old functions from tools.pdb, which I think were intended for or once upon a time used for generating edge features, but is now done in feature.atomic_contact

closes #207 and #218

[DaniBodor]

All tests run fine locally, not sure why they fail here

[gcroci2]

Don't we need type_ var?

[cbaakman]

It's used for getting the right vanderwaals parameters from the table file.

[DaniBodor]

As far as I could see it was only used internally by the get_vanderwaals_parameters function. However, this function basically did nothing else than read the type_ and get the parameters, so instead I merged get_type into get_vanderwaals_parameters.
I tested that old and new add_features functions (from atomic_contact, which uses this) give the exact same result on ~5 test cases, so am fairly confident that it is still being read correctly.

[gcroci2]

Seems good, but the build is failing. Please fix it before merging :)

[DaniBodor]

I noticed that this code is actually slower than the original in assigning features. I tested this by looping some tests from test_query and checking how long it takes to run.

I realize that this is (probably) because before that calculations (e.g. get_coulomb_potentials and get_lennard_jones_potentials) were only run once per graph on many atoms, and now they are run once per edge on few atoms at a time. Apparently the new way is slower.

I can look into finding a way to combine what happened before with the simplified code base, or we can revert to how it was or leave it as is. What do you think?

[gcroci2]

I noticed that this code is actually slower than the original in assigning features. I tested this by looping some tests from test_query and checking how long it takes to run.

I realize that this is (probably) because before that calculations (e.g. get_coulomb_potentials and get_lennard_jones_potentials) were only run once per graph on many atoms, and now they are run once per edge on few atoms at a time. Apparently the new way is slower.

I can look into finding a way to combine what happened before with the simplified code base, or we can revert to how it was or leave it as is. What do you think?

I would just revert it. It would have been a nice addition but I wouldn't lose too much time on this

[DaniBodor]

I would just revert it. It would have been a nice addition but I wouldn't lose too much time on this

I resolved it. Didn't take that long and wanted to use some stuff from this PR anyway.