-
Notifications
You must be signed in to change notification settings - Fork 30
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
ASE support for net charge #58
Comments
ASE is using an array of |
Thanks for having a look @awvwgk . I know what you mean, I am still worried about the inconsistency. Also, as I am using a .xyz file without the charges, this leaves me with no room to correct that with ase/xtb-python interface. Thanks! |
My point here is similar to the argument in #42 and #47 for the multiplicity. Since ASE provides a canonical way to solve this issue, the Keep in mind that in ASE you can actually change the charge of your system after creating the calculator, the calculator must adjust to this change correctly, by adding this property to the calculator it becomes more difficult to keep the information in sync with the geometry used. |
I must have missed the canonical way that you're mentioning, would you mind pointing me towards? I naively assumed that the XTB calculator would a similar charge parameter to the example case. Thanks |
Have a look at the Atoms object constructor which supports:
And |
I have been working with other ase calculators (eg psi4) and can confirm that it is possible to have a net charge for the whole system without having the initial_charges array set. I don't know too much about how this works under the hood, but this is a minimal example to reproduce
It would be cool is the xtb calculator worked the same way |
I asked in the ASE matrix channel and got the following answer:
I'm a bit unhappy about the lack of standardization here, since this means we have to implement our own caching logic for the total charge and multiplicity for the |
For backwards compatibility we should keep the possibility to read from |
Thank you all for the discussion! I also try to get this charge function recently.
from xtb.interface import Molecule,Calculator, Param, Environment
calc = Calculator(Param.IPEAxTB, mol.arrays['numbers'],mol.arrays['positions'],charge=charge,uhf=spin)
calc.set_output('log.txt')
res = calc.singlepoint().get_energy()
we can use similar codes in the psi4 API: # https://wiki.fysik.dtu.dk/ase/_modules/ase/calculators/psi4.html#Psi4
charge = self.parameters.get('charge')
mult = self.parameters.get('multiplicity')
xyz.info = {'charge':1.0,'spin':0.0}
xyz.info
Out[37]: {'charge': 1.0, 'spin': 0.0} now, you can use _charge = self.atoms.info['charge'] Hope those can help! It would be cool to have this function! |
A simple hack to assign net charge for import numpy as np
from xtb.ase.calculator import XTB as OldXTB
class XTB(OldXTB):
"""ASE calculator for XTB with net charge."""
def __init__(self, *args, charge=0, **kwargs):
self.charge = charge
super().__init__(*args, **kwargs)
def _create_api_calculator(self):
initial_charges = np.zeros(len(self.atoms))
initial_charges[0] = self.charge
self.atoms.set_initial_charges(initial_charges)
return super()._create_api_calculator() |
Describe the bug
Directly using XTB-python interface the charge= parameter can be passed, but employing the same approach with ASE/XTB, charge doesn't get propagated
Ie, this works for charge=0 and charge=1 and changes energy accordingly
However, this doesn't remember the charge and always sets charge to 0:
To Reproduce
unzip the file and run the script
f.zip
Expected behaviour
charge affecting the final energy when ase/xtb is used
The issue seems to be in the XTB class :
_charge = self.atoms.get_initial_charges().sum()
Thanks!
The text was updated successfully, but these errors were encountered: