POTCAR modifications ignored in elastic workflow

iicurtis commented 7 years ago

System

Version: master
Python version: 3.5.3
OS version: fedora 25

Summary

Modifying the vasp_input_set as recommended here is ignored after the workflow is generated.
PAW_PBE Sm_3 07Sep2000 is used by default, I would like to use PAW_PBE Sm 21Nov2014

Example code

from pymatgen.io.vasp.inputs import Poscar
from pymatgen.io.vasp.sets import MPStaticSet
from atomate.vasp.workflows.base.elastic import get_wf_elastic_constant
from fireworks import LaunchPad

lp = LaunchPad.auto_load()
poscar = Poscar.from_file("POSCAR")
structure = poscar.structure
vis_input = MPStaticSet(structure)
vis_input.config_dict['POTCAR']['Sm'] = 'Sm'   # Changed here from Sm_3
wf = get_wf_elastic_constant(structure, vasp_input_set=vis_input)
lp.add_wf(wf)

Error message

N/A

Suggested solution (if any)

It seems to be passed correctly all the way to TransmuterFW. I'm not sure what causes it to be modified later.
If no solution is easily available, is there an equivalent to add_modify_incar (but add_modify_potcar or similar) that can work as a post-fix?

iicurtis commented 7 years ago

Fixed via #144 which adds add_modify_potcar functionality to post-fix the work flow.

from pymatgen.io.vasp.inputs import Poscar
from pymatgen.io.vasp.sets import MPStaticSet
from atomate.vasp.workflows.base.elastic import get_wf_elastic_constant
from atomate.vasp.powerups import add_modify_potcar
from fireworks import LaunchPad

lp = LaunchPad.auto_load()
poscar = Poscar.from_file("POSCAR")
structure = poscar.structure
vis_input = MPStaticSet(structure)
wf = get_wf_elastic_constant(structure, vasp_input_set=vis_input)
vis_input.config_dict['POTCAR']['Sm'] = 'Sm'   # Changed here from Sm_3
potcar_symbols = vis_input.config_dict['POTCAR']
wf = add_modify_potcar(wf, modify_potcar_params={"potcar_symbols": potcar_symbols})
lp.add_wf(wf)

computron commented 7 years ago

@matk86 can you look why the original code didn't work?

matk86 commented 7 years ago

thats because msonable as_dict serialization takes into account only the args to the init...overriding inputset by accessing confgi_dict attribute(or any other attribute for that matter) is not reflected in the serialization...in atomate, the serialized version of the given input set is used in WriteTransmutedStructureIOSet to override the 'structure' (set to the transformed structure)

matk86 commented 7 years ago

DictSet in pymatgen only enables setting potcar_fuctional(can be set via the inputset kwargs)

computron commented 7 years ago

@shyuep is there any reason for VaspInputSet to not serialize the config dict?

Here is my understanding of the situation based on @matk86 and talking to @montoyjh

If you re-load a VaspInputSet class using as_dict and then from_dict, the "config_dict" parameter is lost in that procedure. It would seem to me that config_dict should be retained during this procedure.
The TransmuterSet performs this dump/restore because of the fact that the VaspInputSet is designed to be tied to a particular structure. The VaspInputSet the user provides is for the initial structure. After transmuting the structure into a new structure, a new VaspInputSet must be createad for that new structure. To get this new VaspInputSet, the TransmuterFW is doing the dump/restore of the original vaspinputset but with the structure replaced. Unfortunately, config_dict is lost in this process. @montoyjh and I think it would be better if VaspInputSets didn't require a structure for initialization, only when calling the get_inputs function.

Although we can probably patch this in TransmuterFW, e.g., by manually copying the config_dict after dump/restore, it's probably better fixed in pymatgen. What do you think?

computron commented 7 years ago

Btw the relevant pain point in atomate is: atomate/vasp/firetasks/write_inputs.py:334

montoyjh commented 7 years ago

As @computron mentioned, I'm also of the opinion that vasp input sets shouldn't require a structure for initialization, but I think changing that would require a pretty significant refactor. A more practical short-term solution might be to just have a "user_potcar_settings" that's absolute, similar to "user_incar_settings" and "user_kpoints_settings".

shyuep commented 7 years ago

There are many good reasons why the config dict should not be serialized.

That is a big-ass dict. Since in most cases it is repeating data, it is pretty bad to just blow up the json and storage just for these purposes. i.e., this is low information entropy. We should encode using the least bits.
Most things can be achieved via user_incar_settings, and that is definitely serialized.
The whole idea of a InputSet is to standardize. If people modify the dict extensively, there really isn't a point to it is there? Generally if you are going to completely change the PSP set, you really should be writing a new InputSet (e.g., MP vs MIT relax sets). Note that changing PSP has profound implications, not least in terms of compatibility. It is not a good idea to allow people to change the PSP and forget about it.

Like what @montoyjh said, you can add user_potcar_settings and user_kpoint_settings. Or perhaps even more generally, a user_config_dict_settings (though the usage is highly complex). But before I agree to such changes, I really need to understand (a) the use case, and (b) the consequences. Also, what cannot be achieved by simply a MyOwnSet(MPRelaxSet) that overrides the required PSP.

Finally, we actually had VaspInputSets that do not take structure in pmg < 4, and we explicitly refactored to one that did. The main reason here is that the actual structure has an effect on all inputs - INCAR, KPOINTS, POSCAR, POTCAR. There are many times you want the input set to perform some pre-processing of the structure (e.g., in the BS, you want to go to the standard cells). You can either say that the incar, kpoints, poscar, and potcar generation methods all need to call a "get_standardized_structure" before they generate the input, and this is repeating the same call in multiple locations, and some idiot coder can always forget to include such a call, resulting in hard to find bugs. Or you can simply ask that people supply a structure in the init and any standardization is done there and all the other methods simply use the same structure. Supply structure also allows people to supply history information, e.g., prev_incar etc. Given that the cost to initializing input sets is low, I think this is a necessary compromise for greater code reliability. You will notice that since we switched, we seldom had the bad bugs we had in the past.

computron commented 7 years ago

Can we just add user_potcar_settings then? I think the reason to "hack" the config_dict in the first place was because this was missing

shyuep commented 7 years ago

That is fine by me for now. But please issue a warning when user_potcar_settings is set. Generally, the modification of INCAR and KPOINTS is not as severe as modifying POTCARS for compatibility and many other analysis. This should be implemented in the DictSet so that it will propogate to all children sets.

If something is meant to be used more than once, I would definitely recommend writing a separate InputSet (which is probably less than 10 lines of code) to be explicit that this is different from your MP sets.

shyuep commented 7 years ago

While the person is at it, I think it is best to change config_dict to _config_dict to discourage meddling.

computron commented 7 years ago

Ok I am going to close this issue on atomate and a feature request to PMG

shyuep commented 7 years ago

I pushed the fix.

hackingmaterials / atomate