Atomate v2: consistent signature for Firework constructor arguments

[computron]

For atomate v2, our discussions indicate that we want a consistent signature for the initial few arguments of any Firework. Following this consistent signature, users can choose to add additional arguments as necessary.

The arguments to a Firework should be:

• structure - can be a Structure (or list of Structures), or it can be a filename like CONTCAR (or list of filenames). We discussed at length the problems of requiring a Structure object only, since many times one does not know the structure of a downstream Firework in advance.
• inputset - this can a String name of an input set (e.g. "MPStaticSet"). There were reasons for the String - including the necessity for VIS objects to have a Structure (sometimes difficult) and also for cleanliness of JSON in to_dict(). @bocklund I think you also wanted to be able to put a full-fledged VaspInputSet object here (not just a String). But VIS objects require a structure in advance. How does this work if the Firework is used in the middle of the workflow, and the structure argument to the VaspInputSet must be loaded at the time of workflow execution (not at the time of workflow creation)? It does not seem trivial to simply change the structure of an existing VaspInputSet object for which the structure was already loaded, without doing something clunky like exporting to dict, overriding the structure, and converting back to object.
• inputset_params - this is a dictionary of settings that is passed directly into the constructor of the input set as kwargs. Thus, one would initialize an inputset for the Firework at the time of execution of the Firework (when the structure is known) using the class listed in inputset, the structure listed in structure, and the arguments used in inputset_params.
• copy_prev - if set to True, this will generally add a FireTask in the beginning that copies VASP outputs from the previous Firework to the current one. Users will set this true when this Firework is being used in the middle of a chain.
• override_prev - if set to True, this will use the override_from_prev_calc() function in the VASPInputSet to write the VASP inputs. We discussed combining this with the copy_prev parameter, but several people wanted to keep this separate.
• name - give the user the option to override the name of the Firework (same as before)

Following these parameters, the developer could add more arguments if desired. But the parameters above would always be present in all VASP atomate Fireworks and in a consistent order.

Thoughts / comments? This is an important decision.

[mkhorton]

My comments on constructor arguments are purely from improving ease of use for a beginner, and ease of teaching.

• inputset_params should be unfolded, i.e. so that DictSet kwargs like user_incar_settings etc. are keyword arguments to the FW themselves. Trying to get people to understand the format of this dictionary and what keys should be present is always difficult

• structure should be a kwarg, and raise a ValueError if both structure is provided and copy_prev is set

• inputset as String -- my only concern here is if someone defines a custom input set not inside pymatgen.io.sets

• not really related to the constructor signature, but we should do more error checking: if e.g. flags in user_incar_settings are set that are not valid we should tell the user (this improvement can be done in pymatgen directly)

Regardless of these points, I'd like to see this enforced in a VaspFW base class so that it's easier to make common modifications after the fact. Right now we're in a very inconsistent state between different fireworks.

[shyuep]

Input set can be specific as a FULL string. We regularly write these as mylab.sets.MyOwnSet. It is already the way it is implemented in the MontyDecoder. These will allow people to write their own input set if they wish. Of course we can allow a default in which MPRelaxSet means the version in pymatgen.io.vasp.sets.

[computron]

• I agree that custom input sets are not a problem - e.g., full path to class.
• I don't like the idea of adding even more arguments (i.e., repeating all the kwargs arguments of DictSet, of which there are 14!!) to the fireworks signature. I think it's enough to have just one. I agree it will be a little more teaching to newbies to let them know that inputset_params can take in anything a VaspInputSet can take, which includes any of the 14 inherited kwargs from DictSet. But this is knowledge they should have anyway - it's good for them to know that kwargs from DictSet can also be set in any VaspInputSet.
• For structure, the idea is:

• If it's the first Firework in the workflow, you set the structure equal to a Structure object or list of Structures
• If it's a "typical" downstream Firework, you set structure equal to "CONTCAR" and copy_prev=True
• If it's an "atypical" downstream Firework, e.g. one with multiple parents, you may need to do something atypical - like set the structure=["CONTCAR.1", "CONTCAR.2"] and use an atypical FireTask to copy runs from the multiple parents and append the ".1" and ".2" suffix.

I agree it would be nice not to have to set the structure at all for downstream Fireworks, this was a major pain in atomate v1. I was thinking that the default for structure could be None, and None would (i) throw an error if copy_prev was False or (ii) set to "CONTCAR" (or similar depending on the Firework) if copy_prev=True.

There might be some odd reason for someone to want to explicitly specify a structure yet still use copy_prev=True. As a silly example, maybe they want to see the OUTCAR from the previous job to do some analysis and make a decision about something, but know that the structure they want to inject in advance (don't want to use the structure from that previous job). So I wouldn't necessarily throw an error if someone uses copy_prev=True and also gives a structure.

[JosephMontoya-TRI]

I like all of these.

[computron]

Ok I think we have general agreement here, we will use this as a starting point for atomate v2.

Thanks all for participating in the discussions!

[bocklund]

@computron I think with the new changes in atomate v2, VIS objects will not be necessary for us as long as full string import paths are supported.