It seems that here an instance of IDPPSolver is returned. I think that the output of this function should be the output of the IDPPSolver.run() method.

Good catch! Will add some logic for separating out the IDPPSolver.run and constructor kwargs

I have seen that the pymatgen interpolate method behaves in a somewhat weird way: nimages+1 structures are returned: https://github.com/materialsproject/pymatgen/blob/f9d9fe8e0ce09ef30cc03bcc4e9937d27afd5a6a/src/pymatgen/core/structure.py#L2443
So you get a set of structures which is not nimages in total nor nimages plus the initial and final structure. Which I found quite unexpected.
In addition this is at variance with what the IDPPSolver does. In that case a total of nimages+2 structures is returned.
So the meaning of nimages varies depending on the inteprolation method.

I would suggest passing num_images+1 to interpolate. This would maintain consistency among the methods and would also seem more reasonable.

Not sure if this is supposed to be propagated to the NebSetGenerator or if it is a leftover of a previous option, but it looks like it is not used anywhere.

It was a holdover from earlier logic, thanks for the catch

it may be good to add the docstrings for this option, as its use is quite obscure without looking at the code.

Good point, I'll move it into the main docstr (the explanation was buried in the maker) - will also change the default behavior to use this fix

I did not try to do the math myself to see what the problem could be with this point, but in one of my tests I had an issue. The optimizer considered the forces converged and stopped the optimization after few loops, but this check was False and thus the task was marked as failed.
More in general, is there a need for this check specifically? optimize.run already returns a bool to specify if the optimization converged or not, why not using that?

Probably has to do with the difference in NEB forces (interatomic + spring) vs plain interatomic forces. I'll take this out - I like the idea of using optimize.run to determine the task state, I'll update the other jobs as well

This is a generic comment on the NebFromImagesMaker, but I wanted to mention that I found somewhat confusing the fact that this job returns a NebTaskDoc, because by construction this will only contain the energies of the intermediate images and not those of the endpoints. As a consequence all the barrier analysis and values reported in the output document are wrong.
Even assuming that this is always used inside the NebFromEndpointsMaker the output database will contain two NebTaskDoc coming from the same flow, one of which is incomplete.

I am afraid I don't have a much better way of proceeding to propose. One potential suggestion would be that the output of this job should only be a minimal document with the list of folders and some information about the vasp calculation, relying on the fact that a collect_neb_output Job will be run as a subsequent step.

Hmm, maybe there should be an NebIntermediateImageResult class that has this minimal info. Wouldn't need to be in emmet since this is just for book-keeping on the atomate2 side

I don't know if it was just because my example was too trivial, but with the default EDIFF the forces were not converging even for a large number of steps, probably due to the SCF not being properly converged. It instead converged was quite fast as soon as I lowered EDIFF. I don't have many test cases, but if you also encountered this issue it may be worth considering using a lower value as a default.

Always hard to say with NEB but EDIFF = 1e-6 or 1e-7 might be a safer default

Maybe I am missing something but it seems that with set_type="image" the INCAR containts ISIF=2 and thus volume is not changing. So what is the point of the double relaxation here?

This is mostly for continuity / reproducibility of the original atomate version. Might be good to have a legacy / current split of the input sets, just like we do for the EOS workflows

It would be good to have a check here to verify that inserted_coords_dict and inserted_coords_combo are consistent. I had a typo in my inserted_coords_combo and this caused an error only late in the Flow execution, with no obvious link to the cause of the error.

Better specify that these should be fractional coordinates. I had to look at the code to know if they had to be fractional or cartesian.

It is mentioned in the docstrings, but I would add a check that only two structures are actually passed in the end_point_structures.
In principle I suppose there would be no particular issue implementing this Flow accepting more than 2 endpoints and calculating all the subsequent jumps. Before checking the docstrings I was expecting it would have been possible to pass more than 2.

Maybe it is redundant with the other check I am suggesting in CommonApproxNebMaker, but in case this Job ends up being used outside that flow it may be good to make a check here as well. In this loop if one of the keys in ep_distinct is not endpoint_coords the code will just pass without creating the relaxation job for that endpoint, but fail later on.

Having both is a good fallback

Sorry, I just realized that here and in all the other jobs the schema is set through the schema argument, but it should be output_schema. Jobflow does not raise an error since a job accepts kwargs.