BUG: Should prev job INCAR be merged into next job

[janosh]

The new MP r2SCAN workflow in #362 defaults to a PBESol pre-relax followed by the r2SCAN relax. Currently atomate2 merges the previous job's INCAR into the next one in the VaspInputGenerator.

atomate2/src/atomate2/vasp/sets/base.py

Lines 402 to 411 in ff60708

	structure, prev_incar, bandgap, ispin, vasprun, outcar = self._get_previous(
	structure, prev_dir
	)
	incar_updates = self.get_incar_updates(
	structure,
	prev_incar=prev_incar,
	bandgap=bandgap,
	vasprun=vasprun,
	outcar=outcar,
	)

This means GGA: PS from the PBESol's INCAR ends up in the r2SCAN relax INCAR together with the METAGGA: R2SCAN that's there anyway. Up until VASP 6.3.2, that's fine since the METAGGA takes precedence. Starting in 6.4, VASP raises an error if GGA and METAGGA are set simultaneously. I think it's worth adding having the ability to disable INCAR inheritance on a case by case basis and/or change the default behavior to not inherit. I think a previous job's INCAR affecting the next job goes against the principle of least surprise for new atomate2 users.

[janosh]

Forgot to mention, one potential upside of this default is that any Custodian corrections applied to the INCAR will trickle down to follow up jobs. But I think this can swing both ways. It could help a follow-on job succeed but it could also make it fail or give erroneous results if the correction doesn't apply e.g. due to change of functional.

[utf]

Yes, inheritance is definitely expected behaviour. This is how atomate1 worked, and it has the benefits of:

1. Custodian fixes propagating through calculations.
2. Being able to configure settings for the first job (e.g., determining KSPACING through convergence tests) and then propagating these values to subsequent calculations.

If you need to set GGA to a certain value for r2SCAN to run properly then this should be set in the input set generator. E.g., in this case you'd need to set GGA=None in the get_incar_updates function.

However, whether this is something we should continue doing I'm not so sure. It can definitely add confusion, and it adds a lot of logic to the input set generators that could be removed if we disabled inheritance. On point 2: the standardised logic in atomate2 for applying input set updates means that it is easier than ever to configure inputs for all jobs (previously this was a bit more effort in atomate1). So perhaps inheritance is not necessary.

That said, I wouldn't minimise the benefits of inheriting custodian fixes. It can waste a lot of CPU cycles finding an ALGO that gives convergence. As this would be a major change to how atomate2 works, I'd be interested to hear other people's thoughts. In particular, as far as I'm aware, quacc doesn't use inheritance. Have you found any downsides to this @arosen93?

[Andrew-S-Rosen]

@utf. This is a very good question. I haven't done a detailed comparison, but at the same time I haven't had major issues by not carrying flags over (that I'm aware of --- these things can be subtle). I think this is something that likely needs further detailed investigation. There are definitely pros and cons.

That said, part of me wonders if this logic makes more sense to have within Custodian. For instance, maybe a 'prior_dir' kwarg could be added and then it would check to see what fixes were applied in the prior run. Then it would be easy to adjust whether inheritance (of fixes) is used via a global Atomate2 setting that could be set to True by default. This would just carry over the fixes, not the full list of INCAR flags, so from a workflow-writing perspective things may be a little bit more intuitive. Maybe there are big "gotchas" though.

[janosh]

Another person to ping is @computron. IIRC he expressed an opinion on INCAR inheritance at the atomate2 maintainer meeting.

[janosh]

E.g., in this case you'd need to set GGA=None in the get_incar_updates function.

@computron Thanks for the advice. Done in c98fe98.

[utf]

Great. Maybe this is a topic to discuss at the next atomate2 meeting. It would be great if you could post the outcome of the discussion on this thread.

[janosh]

@utf I brought this up in the last atomate2 meeting where we didn't come to any firm opinions because we were lacking the full background you posted above. Now that we have that we can discuss again next meeting and see what everyone thinks.

I'll offer my opinion upfront that I think INCAR inheritance should not be the default so as not to surprise users. It should either be a boolean flag in the input set generator or maybe even better also support a tuple incar_inherit: bool | Sequence[str] which people could set to e.g. incar_inherit = ('ALGO',).

[Andrew-S-Rosen]

We might also consider adding it as a global config setting.

[utf]

I would be open to that idea. But I would like to hear what other users of the VASP workflows think about removing inheritance. For example @JaGeo (when she is back in the office).

[Zhuoying]

Very insightful discussions. The next atomate2 monthly meeting is on 08/25 (11 am PST).
We will bring the discussions under this thread to the table again. @utf You are absolutely welcome to join if you are available.

From a VASP workflow user's perspective, in my humble opinion (after knowing all the background):

Benefits I want to keep on incar inheritance:

1. ALGO optimized from prev_dir by Custodian as @utf mentioned
2. Any ISMEAR=-5 (tetrahedron method) failed runs that have been updated in prev_dir. (not very sure if Custodian can fix it and bring this incar flag to the next runs in the current atomate2 version).

Problems that might be caused by inheritance :

1. This GGA=PS and MetaGGA conflict issue starts from the latest VASP (6.4), as @janosh posted here.
2. Some INCAR tags save extremely large files (CHGCAR, WAVECAR), which may increase data size rapidly without the user's awareness.
3. Future confusion and hidden errors similar to 1.

What @arosen93 said, "This would just carry over the fixes, not the full list of INCAR flags" sounds super reasonable to me (although it may take more effort to discuss which tags/fixes by Custodian in INCAR should be inherited or not).
This is more like a strategy problem that will constantly come up to new users and new developers for new workflows, thus I personally think it is worth more discussions and time invested.

[computron]

We discussed this at the atomate2 meeting on 8/25/23.

The universal (~6 people) consensus was that they do not want INCAR inheritance by default. I agree with this as I find that when developing workflows, the principle of least surprise is one of the most important principles to abide by. Otherwise, we end up with 100,000 DFT calculations, all of which are entirely useless, because someone tried to be clever and automatically "help" the user by fixing their parameters for them in a way they did not request.

Thus, if I don't hear big objections in the next week, I say we turn off INCAR inheritance by default.

Next was whether to have optional INCAR inheritance, and what what form it would take if we introduced optional inheritance. At the meeting I asked when people would want to switch this on if they had the option, and didn't get any takers. I personally could see some argument for doing this in double relaxation jobs which are now split up into two jobs rather than one, but it wouldn't even necessarily be a strong preference. We are looking for opinions about how much time could potentially be saved by inheriting parameters rather than just starting from scratch. Some considerations:

• Time is lost (not saved) if a downstream job which would have converged fine with ALGO=Fast now takes a long time because it inherits an ALGO=Normal tag from an upstream job.
• Time is barely saved if a downstream job would have quickly failed with its initial parameters and fixed with custodian anyway
• Time is significantly saved if inheriting parameters prevents a long and convoluted process to get to the custodian fix needed to get a downstream job to converge

Do we have evidence (informal is fine, anything really) of significant time savings by inheriting custodian fixes between jobs? This usually means inheriting parameters between jobs of vastly different types, e.g. a static job inheriting things from a relaxation job.

Finally, a note that @arosen93 brought up only inheriting fixes from the custodian.json rather than the INCAR itself, but before going into this complexity it would be nice to know how useful this is.

[matthewkuner]

In my experience, I have found the current inheritance to be a minor negative, as it has sometimes overwritten settings I wanted to keep. This has caused me to resort to just using the user_incar_settings for every job in most of my workflows. Not a big deal either way, though removing the broad inheritance may make maintenance easier. Perhaps only inheriting the Custodian handler changes would be a good middle-ground?

[esoteric-ephemera]

This is actually a huge problem - apparently VASP will use both GGA and METAGGA tags, at least in some versions (6.3.2 in the attached example).

There are three subdirectories in the attached, all for the same sapphire Al2O3 structure taken from MP: one using (ISMEAR = 0, METAGGA = R2SCAN), one using (ISMEAR = -5, METAGGA = R2SCAN), and the last using (ISMEAR = -5, GGA = PS, METAGGA = R2SCAN) (ISMEAR_-5_gga)

The crazy thing is that specifying GGA = PS, which happens in our R2SCAN WFs from prev_incar inheritance, increases the total energies in OSZICAR by an order of magnitude

I'll also add that this behavior is consistent across >1,000 structures that I repeated this test for
mp-1143_r2SCAN.tar.gz

[utf]

Thanks for the discussion, I'll try to response to @computron soon but in general I agree.

But just to reply to @esoteric-ephemera. If GGA needs to be unset for a R2SCAN calculation, then this should be specified in the input set get_incar_updates function. Just how a static calculation is defined by NSW=0. So I would say that this is a bug in the WF not an issue with inheritance.

[esoteric-ephemera]

@utf I'd argue this shows that inheritance isn't the best choice, because we did not want both tags specified, but the inheritance propagated the GGA tag

[utf]

I agree that inheritance adds extra complexity for developers and users. But I was just responding to your point that this is a huge problem. It isn't a problem, provided the input sets are written with inheritance in mind.

[Andrew-S-Rosen]

I think he meant it's a huge problem for MP as it stands right now... 😅

[utf]

Inheritance was disabled in #594