Skip to content

Conversation

samwaseda
Copy link
Member

Copy link
Member

@jan-janssen jan-janssen left a comment

Choose a reason for hiding this comment

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

Is not the parent=True option pyiron specific? The goal for the structuretoolkitis to be completely based on ASE Atoms.

@samwaseda
Copy link
Member Author

Is not the parent=True option pyiron specific? The goal for the structuretoolkitis to be completely based on ASE Atoms.

Ah good point. Let me think about it.

@samwaseda
Copy link
Member Author

Do you know whether it's actually possible to change the chemical symbols in ASE? I take a quick look at it and I couldn't find it.

@jan-janssen
Copy link
Member

Do you know whether it's actually possible to change the chemical symbols in ASE? I take a quick look at it and I couldn't find it.

I guess the only special element they have is X:
https://gitlab.com/ase/ase/-/blob/master/ase/data/__init__.py#L12

from ase.atom import Atom
from ase.atoms import Atoms
structure = Atoms([Atom("X")])
print(structure.get_positions(), structure.cell)

@jan-janssen
Copy link
Member

For changing elements you can use:
https://github.com/pyiron/structuretoolkit/blob/main/structuretoolkit/common/helper.py#L130

structure.symbols = element_lst 

@samwaseda
Copy link
Member Author

That means the fundamental problem is the fact that we allow any element name then, which is then used in get_chemical_symbols.

@jan-janssen
Copy link
Member

That's why I use pyiron_to_ase() in pyiron/pyiron_atomistics#1053 but I agree pyiron to ASE currently just ignores extra elements.

@samwaseda samwaseda changed the title Add parent to get_chemical_symbols Use get_atomic_numbers instead of atomic_numbers May 25, 2023
@jan-janssen
Copy link
Member

I have to admit I was not aware that ASE had an get_atomic_numbers() function. But in this case I guess we can remove:

def get_atomic_numbers(structure):
    return structure.get_atomic_numbers()

And instead replace the get_atomic_numbers(structure) calls with structure.get_atomic_numbers().

@samwaseda
Copy link
Member Author

I have to admit I was not aware that ASE had an get_atomic_numbers() function. But in this case I guess we can remove:

def get_atomic_numbers(structure):
    return structure.get_atomic_numbers()

And instead replace the get_atomic_numbers(structure) calls with structure.get_atomic_numbers().

I wasn't either :D but I guess then we found the optimal solution

@samwaseda
Copy link
Member Author

sorry a mistake, just a moment

@jan-janssen
Copy link
Member

sorry a mistake, just a moment

I am happy - at least the tests failed ;-)

Copy link
Member

@jan-janssen jan-janssen left a comment

Choose a reason for hiding this comment

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

Looks good to me - feel free to merge it once the tests pass

@samwaseda samwaseda merged commit a25b332 into main May 25, 2023
@samwaseda samwaseda deleted the get_chemical_symbols branch May 25, 2023 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants