jkitchin / jasp

python enhancements of ase.calculators.vasp
Other
27 stars 13 forks source link

Changing the atoms object after the fact does not lead to restart or error #41

Closed prtkm closed 9 years ago

prtkm commented 9 years ago

Changing the metal from 'Pd' to 'Pt' will change the atom symbol in calc.atoms, but not restart a job. Changing the unit cell params will not change the unit cell in calc.atoms.

It's strange because there is a check here: https://github.com/jkitchin/jasp/blob/master/jasp/jasp_extensions.py#L317

Something probably changes the atoms in jasp.py. The unit cell issue can probably be explained by https://github.com/jkitchin/jasp/blob/master/jasp/jasp.py#L333

Sample code to reproduce, by changing b and 'Pt':

#+BEGIN_SRC python
from ase import Atoms, Atom
from jasp import *
import matplotlib.pyplot as plt
import numpy as np

a = 3.9  # approximate lattice constant
b = a / 2.
bulk = Atoms([Atom('Pd', (0.0, 0.0, 0.0))],
             cell=[(0, b, b),
                   (b, 0, b),
                   (b, b, 0)])

with jasp('bulk/pd',
          encut=520,
          xc='PBE',
          kpts=(20, 20, 20),
          debug=True,
          atoms=bulk) as calc:
    bulk.get_potential_energy()
    print bulk.calc
jkitchin commented 9 years ago

This might be expected behavior. When jasp loads, it replaces the atoms you put in with the atoms it reads from the directory. You have to change the atoms inside the context manager to trigger a calc required. Does that work?

prtkm commented 9 years ago

setting calc.atoms with a changed symbol inside the context manager seems to resubmit the job, but doesn't actually change the atom in the POSCAR or POTCAR. In any case, I can't imagine a situation when one would try to change the atoms object in the context manager.

However, changing the initially defined atoms object might be a more common mistake made by students/beginners. I actually had a student who somehow tried to change the symbol of the atoms, and forgot to change the directory name, so his 'O' calculation was actually an 'Ar' calculation.

'print calc' printed the atoms object as having an 'O' atom though the energy and the other results were clearly those from Ar. Obviously this can potentially be misleading, especially to people doing their 1st or 2nd calculations.

I'm not sure what the right way to resolve this is though.

jkitchin commented 9 years ago

I pushed some changes that might resolve some of this. basically, it should now raise an exception if the atoms you pass in do not match what exists in the directory in length or symbols.

I think I agree with your first point.

I think the pushes will prevent the second and third issues.

hopefully I did not introduce any new issues ;) Let me know if this works for you.

prtkm commented 9 years ago

Thanks, that seems to work. It's probably a good idea to check if the atoms object in the directory and the passed atoms are equivalent. The current implementation will not catch changes in unit_cell for e.g.

You have this cool function that could be implemented here: https://github.com/jkitchin/jasp/blob/master/jasp/jasp_atoms.py#L9

jkitchin commented 9 years ago

Prateek Mehta writes:

Thanks, that seems to work. It's probably a good idea to check if the atoms object in the directory and the passed atoms are equivalent. The current implementation will not catch changes in unit_cell for e.g.

This is the kind of thing where you have to choose what takes precedent. jasp chooses the directory as the latest. that makes jasp work the way I expect, e.g. you get the relaxed geometry inside the context manager, not whatever geometry got passed in.

but, changing number of atoms or atom types cannot be reconciled, and it is almost surely an error to try that. so it isn't quite the same equality as in jasp_atoms.py#L9.

You have this cool function that could be implemented here: https://github.com/jkitchin/jasp/blob/master/jasp/jasp_atoms.py#L9


Reply to this email directly or view it on GitHub: https://github.com/jkitchin/jasp/issues/41#issuecomment-84467926

Professor John Kitchin Doherty Hall A207F Department of Chemical Engineering Carnegie Mellon University Pittsburgh, PA 15213 412-268-7803 @johnkitchin http://kitchingroup.cheme.cmu.edu

prtkm commented 9 years ago

Ok. yes. That makes a lot of sense. I wasn't thinking about relax jobs! Thanks!

prtkm commented 9 years ago

This line (https://github.com/jkitchin/jasp/blob/master/jasp/jasp.py#L266) will lead to an UnboundLocalError: local variable 'calc' referenced before assignment if jasp is run on a job in the queue.

jkitchin commented 9 years ago

Doh. Will it work to put patoms there?

On Monday, March 23, 2015, Prateek Mehta notifications@github.com wrote:

This line (https://github.com/jkitchin/jasp/blob/master/jasp/jasp.py#L266) will lead to an UnboundLocalError: local variable 'calc' referenced before assignment if jasp is run on a job in the queue.

— Reply to this email directly or view it on GitHub https://github.com/jkitchin/jasp/issues/41#issuecomment-84886433.

John


Professor John Kitchin Doherty Hall A207F Department of Chemical Engineering Carnegie Mellon University Pittsburgh, PA 15213 412-268-7803 @johnkitchin http://kitchingroup.cheme.cmu.edu

prtkm commented 9 years ago

Yep, that fixes it :)

jkitchin commented 9 years ago

I just pushed that.