Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consistent atoms order #338

Merged
merged 6 commits into from
Sep 18, 2021
Merged

Consistent atoms order #338

merged 6 commits into from
Sep 18, 2021

Conversation

sudarsan-surendralal
Copy link
Member

Create a consistent order of the atoms and corresponding indices when creating atoms

@sudarsan-surendralal sudarsan-surendralal added the enhancement New feature or request label Sep 2, 2021
@sudarsan-surendralal sudarsan-surendralal linked an issue Sep 2, 2021 that may be closed by this pull request
@pmrv
Copy link
Contributor

pmrv commented Sep 3, 2021

It seems to work, but I'm still a bit stuck on how. Will give it another reading later. :/

EDIT: Also according to what do we want to sort? The examples I gave in #335 sort alphabetically, but the test gives 'Ca', 'Mg', 'Al'.

@liamhuber
Copy link
Member

For maximum safety, shouldn't the test be verifying that species get mapped to the desired cartesian positions?

@niklassiemer
Copy link
Member

It seems to work, but I'm still a bit stuck on how. Will give it another reading later. :/

EDIT: Also according to what do we want to sort? The examples I gave in #335 sort alphabetically, but the test gives 'Ca', 'Mg', 'Al'.

That sorting also does not make sense to me. I would either sort alphabetically or by atomic number.

@coveralls
Copy link

coveralls commented Sep 3, 2021

Pull Request Test Coverage Report for Build 1197462116

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.2%) to 68.295%

Files with Coverage Reduction New Missed Lines %
pyiron_atomistics/atomistics/structure/analyse.py 2 98.9%
Totals Coverage Status
Change from base Build 1182317937: 0.2%
Covered Lines: 11085
Relevant Lines: 16231

💛 - Coveralls

@sudarsan-surendralal
Copy link
Member Author

The random ordering of the species was due to this line:

self.set_species(list(set(el_object_list)))

When you create a set from a list of objects (not symbols) the sorting of the set is random which is why you get the random ordering of the species. My current implementation sorts the species depending on the order of the elements defined and not the alphabetical order. For example, if you define:

struct = Atoms(elements=["Ca", "Mg", "Al". "Ca"], positions=<positions>, cell=<cell>)
print(struct.get_chemical_symbols()

You would get ["Ca", "Mg", "Al"] which makes sense to me. struct.get_species_symbols() would give you the alphabetically ordered symbols ["Al", "Ca", "Mg"].

@pmrv
Copy link
Contributor

pmrv commented Sep 3, 2021

So the order of the original elements?

I'd rather like one fixed mapping between symbols and indices, because if I create multiple structures independently and then put them in an interactive job, structure or training container, I don't want to keep track of this order manually. If we sort alphabetically or by atomic number then it's always clear how to make the indices consistent if you add a structure to an existing set of structures.

I have a working workaround though, so we can discuss on Monday with everyone.

@sudarsan-surendralal
Copy link
Member Author

So the order of the original elements?

Yes.

I'd rather like one fixed mapping between symbols and indices, because if I create multiple structures independently and then put them in an interactive job, structure or training container, I don't want to keep track of this order manually. If we sort alphabetically or by atomic number then it's always clear how to make the indices consistent if you add a structure to an existing set of structures.

I have a working workaround though, so we can discuss on Monday with everyone.

Yeah let's do that!

@pmrv
Copy link
Contributor

pmrv commented Sep 6, 2021

Check in interactive jobs, they solve the same problem.

Copy link
Contributor

@pmrv pmrv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is ok from my end. I might need a method at some point to reorder the indices, but I can add that once I'm there.

@@ -644,6 +644,8 @@ def get_initial_structure(self):
el_list = self.vasprun_dict["atominfo"]["species_list"]
cell = self.vasprun_dict["init_structure"]["cell"]
positions = self.vasprun_dict["init_structure"]["positions"]

# print("vasprun elements", el_list)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: remove dead debug print?

minor cleanup
@sudarsan-surendralal sudarsan-surendralal merged commit 97a1d0b into master Sep 18, 2021
@delete-merged-branch delete-merged-branch bot deleted the atoms_order branch September 18, 2021 07:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to ensure consistent indices when reading from POSCAR?
5 participants