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

structure adapter ase convertor can not deal with symbol with index #1570

Closed

Conversation

unkcpz
Copy link

@unkcpz unkcpz commented Mar 25, 2023

It is allowed in the specification that the symbol of species_at_sites can have an index for the symbol https://github.com/Materials-Consortia/OPTIMADE/blob/develop/optimade.rst#species-at-sites.
However, ase.Atom do not accept this as symbol https://wiki.fysik.dtu.dk/ase/ase/atom.html#ase.atom.Atom.
The function standardize_chemical_symbol is introduced to convert it to the regular specie symbol.

Here is the trackback of failed test:

tests/adapters/structures/test_ase.py:24: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
optimade/adapters/structures/ase.py:91: in get_ase_atoms
    atoms.append(Atom(symbol=species_name, position=site, mass=mass))
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <[AttributeError('atoms') raised in repr()] Atom object at 0x7f1b8cb83d40>, symbol = 'O0', position = [2.4472140189034, 3.0912417710884, 3.985872061678], tag = None, momentum = None
mass = 15.9994, magmom = None, charge = None, atoms = None, index = None

    def __init__(self, symbol='X', position=(0, 0, 0),
                 tag=None, momentum=None, mass=None,
                 magmom=None, charge=None,
                 atoms=None, index=None):
    
        self.data = d = {}
    
        if atoms is None:
            # This atom is not part of any Atoms object:
            if isinstance(symbol, str):
>               d['number'] = atomic_numbers[symbol]
E               KeyError: 'O0'

Copy link
Contributor

@JPBergsma JPBergsma left a comment

Choose a reason for hiding this comment

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

Dear unkcpz,

Thank you for pointing out this issue with the ase adaptor.
The species in optimade can be an arbitrary string, so there is no guarantee that after stripping the digits, you will have a valid element symbol. So I do not think the solution you have here will work. Instead, you would need to read the element from the species.

I'll try to make a hot fix for this issue asap.

@JPBergsma
Copy link
Contributor

JPBergsma commented Mar 25, 2023

I have created a quick fix (PR#1571) and merged it with the develop master branch.
I'll let @ml-evs check it before making a release.

@unkcpz
Copy link
Author

unkcpz commented Mar 25, 2023

Okay, thanks. I close this one.

@unkcpz unkcpz closed this Mar 25, 2023
@unkcpz unkcpz deleted the fix/xx/adapters-ase-symbol branch March 25, 2023 22:45
unkcpz added a commit to aiidalab/aiidalab-widgets-base that referenced this pull request Mar 26, 2023
Using aiida structuredata type will give us more flexibility and control to the structure results get from query.
The queryed OPTIMADE structure will -> aiida structuredata -> ase Atoms.
The ASE structure as the trait of the widget because we want to use its versatile converter.
It can also be fixed by Materials-Consortia/optimade-python-tools#1570 but we need then bump optimade-client and then aiidalab widget base.
Meanwhile, the optimade structure to aiida_structuredata is more robust and versatile to support such as the partially occupied sites.
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.

None yet

2 participants