Feedback

[jrdegreeff]

I went ahead and made a bunch of small changes because that seemed like the fastest way to communicate what I was thinking about. There are no functional changes; everything is either formatting stuff or simplification of certain code to make it more readable and/or more efficient. The substantive changes are in this commit, so I'd focus on that diff rather than the diff of the full PR that has a bunch of unimportant formatting changes from my auto formatter.

Note that I did not read any of the commented out files nor anything in the SNAP directory because I don't know what state they are in currently.

Here are the questions I still have:

• Are the commented out code files (param, bm, coulomb, GaN) dead or just something you are coming back to later?
• I feel like we need to be more careful about how we are using ustrip across this package specificially in nnlist; we seem to make the assumption that all the units are consistent (i.e. ignore them entirely) which I would argue breaks the abstraction of AbstractSystem
• I've never seen the Distances package before, but I'm wondering if it has any functionality to replace your get_distance function. If not, I might circle back to make this function more efficient. I haven't profiled anything (yet), but this is on a hot area (frequently called nested loop) and has a good amount of branching, so it looks like something that could be performance engineered a bit. That being said, if the BallTree initialization bottlenecks, this wouldn't matter much anyway.
• What is the purpose of the i field in the neighbor list? in the current code it seems superfluous to me, but maybe there is another use I'm not aware of.

[jrdegreeff]

Ignore bullet point 3 above. It was simple enough so I went ahead and simplified it into a broadcasted function with just one branch (and tested it extensively to be sure I wasn't changing anything). I don't think we can do any better than than.

[dallasfoster]

I went ahead and made a bunch of small changes because that seemed like the fastest way to communicate what I was thinking about. There are no functional changes; everything is either formatting stuff or simplification of certain code to make it more readable and/or more efficient. The substantive changes are in this commit, so I'd focus on that diff rather than the diff of the full PR that has a bunch of unimportant formatting changes from my auto formatter.

Note that I did not read any of the commented out files nor anything in the SNAP directory because I don't know what state they are in currently.

Here are the questions I still have:

• Are the commented out code files (param, bm, coulomb, GaN) dead or just something you are coming back to later?

This is a good question. Some of the files will definitely come back (simple potentials that I haven't gotten around to changing, i.e., BM, Coulomb), but other code files will have to go through larger refactoring (GaN, which might go away entirely because I think we have to reconsider how we handle multi-element systems that require the combination of interatomic potentials).

• I feel like we need to be more careful about how we are using ustrip across this package specificially in nnlist; we seem to make the assumption that all the units are consistent (i.e. ignore them entirely) which I would argue breaks the abstraction of AbstractSystem

I totally agree. Right now, the BallTree does not accept the unitful types and that is why I strip them before building the neighbor list. The possible solutions are either to: 1) compose a BallTree that accepts unitful types, 2) perform units checking before and reimplement them after building the neighbor list.

• I've never seen the Distances package before, but I'm wondering if it has any functionality to replace your get_distance function. If not, I might circle back to make this function more efficient. I haven't profiled anything (yet), but this is on a hot area (frequently called nested loop) and has a good amount of branching, so it looks like something that could be performance engineered a bit. That being said, if the BallTree initialization bottlenecks, this wouldn't matter much anyway.

• What is the purpose of the i field in the neighbor list? in the current code it seems superfluous to me, but maybe there is another use I'm not aware of.

The main purpose is that, for certain potential types like SNAP, I need a lightweight way to access each of the neighbors of an atom i. The current composition of the code is a hack from a template of how to build a neighbor list (it really just confirms how many neighbors of atom I there are), there are certainly more efficient ways to perform this task.