Lammps refactor species ordering

[Leimeroth]

Refactor the sorting of species to have consisten behavior between different atom styles (see #1353 and #1360)

TODO

• full and bond atom styles

[coveralls]

Pull Request Test Coverage Report for Build 8439517570

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 105 of 108 (97.22%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.08%) to 93.256%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pyiron_atomistics/lammps/structure.py	105	108	97.22%
<!--	Total:	105	108	97.22%	-->

Totals
Change from base Build 8345641982:	0.08%
Covered Lines:	14201
Relevant Lines:	15228

---

💛 - Coveralls

[Leimeroth]

I also modified structure_bond and structure_full. However, I am not sure what half their functionality is supposed to do, so it would be good to if someone has a look. For example in structure_full, there was some molecule id, which however was basically just a counter? I set it to 1, which is what is done in structure_bond. Both actually seem wrong to me. I guess setting everything to 1 should be fine as long as they are undefined, so presumably that could be added via a tag or something.

[jan-janssen]

I also modified structure_bond and structure_full. However, I am not sure what half their functionality is supposed to do, so it would be good to if someone has a look. For example in structure_full, there was some molecule id, which however was basically just a counter? I set it to 1, which is what is done in structure_bond. Both actually seem wrong to me. I guess setting everything to 1 should be fine as long as they are undefined, so presumably that could be added via a tag or something.

That sounds wrong to me. I guess the only example we used so far was simulating water, so in that case you basically count the oxygen atoms and every water molecule - H2O - gets one molecule ID. In your changes all water molecules together just act as one molecule. Is it necessary to touch the bond style at this point? I thought you are primarily using charge.

[Leimeroth]

In your changes all water molecules together just act as one molecule.

This behavior is unchanged from the previous. Before it was set via

id_mol, id_species = 1, el_dict[elements[id_atom]]

so it was permanently 1 for bond and as

indices = self._structure.indices
 for id_el, id_species in enumerate(indices):
                id_mol += 1

so it was basically just the same as lammps index and each atom was a separate molecule. Hence, my opinion that both previous implementations probably did not do what they are supposed to do at all. In fact , I only changed the behavior for full and not for bond. Now both at least to the same.

IMO the cleanest solution would be to check whether molecule indices are defined by the user. Else leaving them on 1 should be reasonable.

[jan-janssen]

Seeing that we do not really have anybody who knows why the current parser is written in the way it is, I wonder if it would be better to replace it with an ASE or pymatgen parser instead. The pymatgen parser is available in https://github.com/materialsproject/pymatgen/blob/master/pymatgen/io/lammps/data.py but from briefly looking at it, I feel the ASE parser would be easier to integrate. Unfortunately, the current ASE version 3.22.1 is not as fully featured as their current master branch, for example the option to write the masses is still missing. So for the moment switching to the ASE parser does not seem to be an option.

[Leimeroth]

With all tests passing I guess this can be merged?

[jan-janssen]

With all tests passing I guess this can be merged?

I had a few stylistic remarks but apart from this it is fine from my side.