Yesterday we ported our major plugin to psi4 and found no problem once we were able to make everything work with the new wave function passing interface. I understand that most changes to the interface are still fluxional, but I wanted to share one idea that I think will simplify the way we handle information passing. One of the complications that followed from the change in the interface is the need to grab wave function pointers and pass it to the next python function. For example, to plot cube files we used to do
This pattern of grabbing and passing the wave function is so frequent that I think should be assumed as the default behavior.
My suggestion is that we introduce a “default slot” where we store the last computed wave function and have each python function check if this is defined. If yes, it will be assume to be the function that contains all the relevant and up to date information. I understand that doing this will reintroduce a python-side version of “process.environment.wavefunction”, but I think that now that we have strictly enforced the return of wave functions on the C++ side it is going to be easier to guarantee consistent behavior on the python side. I am sure the GT people might have already planned this change. I just wanted to open a discussion on it.
I like this idea, because I always like when basic users are able to intuitively use the software. I have no idea how this fits into the planned infrastructure changes, though.
In one sense this wouldn’t be hard to do at all. It would be treating (for the user) wavefunction like molecule is currently treated. That is, the user may activate(mymol) or be confident that energy('hf') acts on the active mol w/o explicitly stating energy('hf', molecule=mymol).
But the wavefunction pass found various cases where modules were misusing P::e by grabbing the same thing from multiple locations that mightn’t have been consistent, trying various things until it worked, etc. It’s all consistent now, thanks to @dgasmith, but I don’t know if we want to give people the temptation to fall back into those ways.
On the other hand, if it’s clearly agreed that P::e.wavefunction() grabbing, like activate(mymol) is strictly for the user and shall never appear either Py-side or C-side (maybe a post-commit hook? I really should learn more about those), I could live with this.
Also, this would also be a step back towards behind-the-scenes-magic-has-granted-you-this-molden-file rather than a more API-like interaction with less preserved in “state”.
So, I’ll chime is as slightly against the proposal and wait to see what others think. Whatever is decided, it should be done before 1.0.0 b/c we don’t to make forseeable changes to the API after that.
What you are suggesting adds more code than what it replaces since each python function now needs to have some additional logic in it. It also obfuscates exactly what system it is being performed on. In my opinion the greatest achievement of the wavefunction pass is making things more explicit, which I will argue is more user friendly. Basically, I’m with @loriab, except I’ll be less PC about it and say I’m against the proposal.
In my coding experience the only good global objects are those that deal with the computing environment.
I am usually strongly in favor of anything that looks simpler for the user. Francesco’s proposal would be simpler for the end user. On the other hand, I am also strongly against global variables, and do feel like this change would be a step backwards on that front.
I wonder if this case could be handled by setting some kind of Psi4 option (cubeprop = true?). In that case, simplicity for the user is maintained without having to mess with the code and add some logic everywhere to look for default wavefunctions. And the syntax
I see the points being made. I think our perspective is a bit different because we have to use this syntax for all our calculations.
If we are going to go with this pattern, then perhaps at some point we should be a bit more careful, and make sure that if cubeprop() and other functions are called without an argument that psi4 returns a meaningful message that explains how to do things properly.
One aspect that I want to point out is that at a fundamental level, in some way were are still handling global (although in this case non-unique) objects. That’s because none of the codes in psi4 use the deep_copy() function (and there are good reasons to do so). So we are effectively still creating shared pointers to the same objects passed from function to function. At this point we still have the potential to garble things up, it’s just very well hidden.
I have added softer fails to the long TODO list for the 1.0 release here. Any help on this list would be much appreciated.
These are not global objects as they must be tied to an object. They can be shared through many objects which may look global if you only have a single derived wavefunction, but this is still quite a bit different than a true global object. For example in SAPT we are now holding and passing up to 7 wavefunctions at a given point in that driver. At the very least, the switch to a deep copy is now somewhat trivial (shallow_copy vs deep_copy).
The wavefunction passing update is certainly not bulletproof, nor was it meant to be. This is merely attempting to step in the right direction to support information passing in a more explicit manner. At this point I’m not sure if we can step backwards without adding a lot of complexity to the driver which would likely create even more holes than already exists.
In general, a shared object is not a global object. The scope of the object is limited to whoever knows about the pointer. Like you said, basically everything is passed around as shared_ptrs in Psi4. I think there is a lot of agreement that shared_ptrs are poorly used at the moment (many of them should be references or at the least unique_ptrs).
The problem you describe (typically called the “aliasing” problem) is not a problem with the shared_ptrs, but rather Psi4’s const-correctness. If we were passing around shared_ptr<const Matrix> then people couldn’t change your variables out from under you.
Sure, I understand and you guys seem to see my point. I was a bit sloppy with my wording, I didn’t mean global as in a global variable/pointer/object. In any case, I very happy with the work you guys have done since we are certainly in a better place than we were before.
We’re probably not going to be able to do very informative soft fails for molden, oeprop, cubeprop (the fns that newly require wfn as parameter) because the error is a TypeError that gets thrown before the function is ever entered. So we’d either have to (1) capture it c-side (which is ugly code to work with https://github.com/psi4/psi4/blob/master/src/bin/psi4/python.cc#L1723) and re-raise or (2) remove wfn from argument list of those fns and then test if wfn in kwargs, emit error message, and fail. The latter plan makes the signature less revealing, but I could live with it.
Ok, there’s got to be some re-factoring of how these things are called. Among property, cubeprop, and oeprop, all of which nominally take a list, we’ve got:
I can’t even think of a fourth way this could have been specified, so it’s lucky molden() only takes wfn. Shall we standardize this? I think I vote for kwarg with name properties, as that advances toward unification.