search

pyiron / pyiron_atomistics

pyiron_atomistics - an integrated development environment (IDE) for atomistic simulation in computational materials science.
https://pyiron-atomistics.readthedocs.io
BSD 3-Clause "New" or "Revised" License
43 stars 15 forks source link

selective_dynamics doesn't appear in the list of tab completion #116

Closed samwaseda closed 3 years ago

samwaseda commented 3 years ago

From what I know even after setting:

structure.add_tag(selective_dynamics=[True, True, True])

selective_dynamics does not appear in the tab completion list, even though it can (or maybe even should?) still be accessed via structure.selective_dynamics. I wouldn't call it a bug but it would be really nice to have it tab-completable.

liamhuber commented 3 years ago

From my perspective selective_dynamics is vestigial VASP-centric syntax that should be purged. From a quick google Vasp is the main (only?) code that uses this phrase, and it's not a native part of the ase.Atoms, but ASE has it tucked away in ase.io.vasp. We don't associate forces, energies, or even velocities with Atoms right now (although for velocities one can make a good case!) so why magic freezing? This should be handled by whatever is doing the evolution.

With that said, we do currently use it, so the complaint it's not in the tab-completion is totally fair. I'd only request that we hide it behind an underscore (my_structure._selective_dynamics) so we're not on the hook for backwards compatibility 😝

sudarsan-surendralal commented 3 years ago

@samwaseda I think the linked PR should fix this

From my perspective selective_dynamics is vestigial VASP-centric syntax that should be purged. From a quick google Vasp is the main (only?) code that uses this phrase, and it's not a native part of the ase.Atoms, but ASE has it tucked away in ase.io.vasp. We don't associate forces, energies, or even velocities with Atoms right now (although for velocities one can make a good case!) so why magic freezing? This should be handled by whatever is doing the evolution.

I see your point and it would eventually make sense to attach this to the calculator rather than the structure. This tag is attached to the Atoms object for historical reasons. The name, even though VASP centric makes sense to me though (maybe I'm biased because I mostly use VASP for atomistic simulations).

With that said, we do currently use it, so the complaint it's not in the tab-completion is totally fair. I'd only request that we hide it behind an underscore (my_structure._selective_dynamics) so we're not on the hook for backwards compatibility 😝

I think a lot of our existing protocols would fail if we change this though so I'm not sure about this! We should at least bring it up in one of the pyiron meetings

samwaseda commented 3 years ago

I guess whether VASP-centric or not is a smaller problem. The bigger problem is that the name selective_dynamics not only does not give you an intuitively clear idea what it means, but also even if you know what it in principle does, it's impossible to know whether True freezes the atoms or False.

samwaseda commented 3 years ago

@sudarsan-surendralal Is there a particular reason that this issue should be reopened?

sudarsan-surendralal commented 3 years ago

The original issue is solved. But I thought the naming and the fact that it is tagged to the Atoms class isn't solved.

samwaseda commented 3 years ago

Ok but that's a different topic so I'm gonna close this one.