kpoints file not overridden for nscf calculation

[dyllamt]

@ardunn and I were running a NSCF calculation and tried to supply our own kpoints file using user_kpoints_settings. The problem is that the kpoints property of the NSCF input set does not listen to this class attribute, because the kpoints property is overridden from the DictSet kpoints property. Basically, the user_kpoints_settings gets ignored.

[shyuep]

This should not be happening. The DictSet.kpoints property respects the user_kpoints_setting as the "ultimate" authority, i.e., if that is set, whatever is written in the input set is ignored. Pls give an example code so that we know what is happening. Note that if you are using from_prev_run for NSCF, that method might be overriding your settings, not DictSet itself.

[shyuep]

I also note that this is everything to do with the implementor of the NonSCFSet and not DictSet itsef.

[dyllamt]

NonSCFSet overrides the kpoints property of DictSet. There is no reference to user_kpoints_settings in this overridden property

[dyllamt]

I started a pull request for this issue
materialsproject/pymatgen#1427

[mkhorton]

We could clean up the pymatgen from_prev_calc API to avoid this kind of issue happening, in particular have a DictSet.from_prev_calc method that enforces things like k-point, incar updates, and make NonSCFSet etc work from that. This might be useful for atomate also to make chaining of arbitrary input set fireworks cleaner.

[dyllamt]

I'm a relatively light user of atomate and the pmg input sets, but I've spent a lot of time trying to trouble shoot issues similar to this in the last month. I think a good permanent solution would be to use decorators on the kpoint, incar, poscar, etc. properties. This way updates to the input set with user_x_settings can always be resolved at the end, so that they are the ultimate authority on the generated input sets.

[dyllamt]

I think that the pmg input set from_prev_calc methods are quite useful, but they make the WriteXFromPrev Firetasks kind of redundant. The extra write Firetasks are a source of confusion, because they create hidden presets from the user (see all of the .get() calls in those Firetasks). All of the Fireworks could use the WriteVaspFromIOSet and just leverage the .from_prev_calc methods

[dyllamt]

There has been some worry that pymatgen controls the default calc settings in atomate (#268). I think this is why the WriteXFromPrev Firetasks exist. If each of the vasp Fireworks included a vasp InputSet in their constructor, we would be able to eliminate these extra Firetasks. The top of the vasp Fireworks module can contain the default InputSets passed to each firework. These input sets will likely be partially generated from the .from_prev_calc() methods in pmg

[mkhorton]

@dyllamt I think a lot of these ideas sound sensible, in particular the decorators. I think the API on the atomate side is currently over-complicated and could be made a lot simpler and couple a lot more closely to the pymatgen sets.

[shyuep]

@mkhorton @dyllamt Just a note that I don't think decorators are the solution to this problem, because it will require all subclassers to remember to put in the decorators. One way is to implement a two-tiered system, whereby the kpoints property calls a get_kpoints abstract method, which is the one that subclassers are supposed to override. But the kpoints property still enforces user_kpoints_settings. But this is a hideous way of coding.

The general principle should be that all subclassers should obey the rules set out in the input set documentation. I reproduce them here. See point no. 2.

The following are UNBREAKABLE rules:
1. All input sets must take in a structure or list of structures as the first
   argument.
2. user_incar_settings and user_kpoints_settings are absolute. Any new sets you
   implement must obey this. If a user wants to override your settings,
   you assume he knows what he is doing. Do not magically override user
   supplied settings. You can issue a warning if you think the user is wrong.
3. All input sets must save all supplied args and kwargs as instance variables.
   E.g., self.my_arg = my_arg and self.kwargs = kwargs in the __init__. This
   ensures the as_dict and from_dict work correctly.