Insertion Worflow optimizefw_kwargs Issue

Atomate
1

override_default_vasp_params cannot be supplied as an input for the argument optimizefw_kwargs in get_ion_insertion_wf (https://github.com/hackingmaterials/atomate/blob/cf04d75ab75ffc565cc160df96c81de751eee4b1/atomate/vasp/workflows/base/electrode.py#L32) because of how the GetInsertionCalcs firetask is written. As it currently stands, the Charge Density Analysis firework will fizzle with the following error:

TypeError: ABCMeta object got multiple values for keyword argument ‘override_default_vasp_params’

due to the following line:
https://github.com/hackingmaterials/atomate/blob/cf04d75ab75ffc565cc160df96c81de751eee4b1/atomate/vasp/firetasks/electrode_tasks.py#L173

I would suggest removing automatically setting NSW=299 because this may conflict with user preferences and is inconsistent with the first structure optimization firework in the workflow. I can make a pull request on github with this change but would welcome other thoughts on how to best resolve this bug.

2

Hey Ann,

I’ve not used this workflow, but I might suggest updating the dictionary before passing it in. This is what is done in mpmorph. Something like this:

kwargs_dict = {"override_default_vasp_params": {"user_incar_settings": {"NSW": 299}}}
kwargs_dict.update(optimizefw_kwargs)
fw = OptimizeFW(
                inserted_structure,
                name=f"structure optimization-{itr}",
                **kwargs_dict,
                )
3

Thanks Eric! I agree that is an option which I can do. I wasn’t really seeing the point of setting NSW but I guess it’s best to leave it if it was put there by Jimmy.

4

Link to pull request with changes: https://github.com/hackingmaterials/atomate/pull/669

5

You may be able to do this in one line using pydash and set_, e.g.

_set(optimizefw_kwargs, “override_default_vasp_params.user_incar_settings.NSW”, 299)

rather than doing the nested if statements, since they can be a difficult to follow.

That said, I agree with you that I’m not sure it makes sense to set NSW to such a high value — I imagine this was necessary in a few cases to let it complete, but maybe in general this is quite high. It depends where you personally want the trade off to lie, between potentially wasting compute but getting better completion rates vs occasionally loosing a good structure. One way to decide this would be to query the insertion database to see how many ionic steps were typically required, e.g. is it common to go all the way up to 299?

Best,

Matt

6

I made a quick histogram on the number of ionic steps (for both relaxations since there are typically two) from 20954 of Jimmy’s insertion calculations. The vast majority fall under 100 steps which I believe is the default NSW value.

Ionic_Steps
Ionic_Steps826×520 14.4 KB

If the max number of corrections hasn’t been reached, doesn’t custodian resume the calculations for another relaxation (with the same NSW value) if the max number of ionic steps is reached? My vote would be to remove this automatic setting of NSW=299 but I’m not sure who gets to make the call.

7

You should definitely propose any changes you want to make in the pull request! It’s up to the package maintainer to approve or reject the proposed changes. If there’s a specific person who wrote a particular section that would be changed, e.g. @jshen in this case, you should let them know in case there is a good reason not to, and then the package maintainer can make the determination if there are conflicting recommendations.

If this is true it’s a moot point anyway, I’m not sure I realized this.

8

So I’m pretty sure the setting at 299 should stay since as @acrutt 's graph shows, the standard value of NSW= 99 is not enough. The graph clearly shows that at least 200 is needed since we have to provide the upper limit of how many steps are required. The extra 1.5X is just the standard fudge factor.
These calculations can start with very bad initial guesses which is why this setting is needed. Any of the proposed ways for fixing the blocking of user assignment values is OK.

9

The relevant custodian error handler/corrective action is in custodian.vasp.handlers.UnconvergedErrorHandler line 831 (sorry for whatever reason this isn’t allowing me to post the link to github)

So if it hasn’t reached ionic convergence it will automatically set IBRION=1 and continue the calculation from the point it stopped at. Therefore if NSW is left as the default (99) and there are no other calculation errors, the maximum number of ionic steps is 495 (since custodian permits a maximum of 5 corrections) which should be sufficient. I don’t think NSW needs to be set so I’ll work on updating the PR accordingly.

10

OK cool, I was not aware of that handler. That does works!

11

Hi all,

Was it decided that NSW goes to 99? If so, that is OK with me.

If the plan is to stick with 299, please let me know as I may weigh in here as I think more evidence needs to be shown to support that (ideally, we want to see success rate, not success count, vs number of ionic steps)

12

I updated the PR to use the default NSW of 99.