-
Notifications
You must be signed in to change notification settings - Fork 139
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add orbital rotation documentation #4729
Conversation
@jptowns does this have what you need? |
docs/intro_wavefunction.rst
Outdated
:name: Listing 1 | ||
|
||
<sposet_collection ...> | ||
<rotated_sposet id="rot_spo"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer name to id.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also missing rotation matrix input in the example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are you referring to by 'rotation matrix input'? This input starts off with no initial rotation. The 'opt_vars' element is only for testing. I could document it, but I don't want to put it in the example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think 'opt_vars' is development only unless loading rotation parameters from optimization parameter h5 is the only supported way. Let first document it but not added to the example considering it is optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think anyone needs to specify initial rotations this way unless it's for testing. For using the results of an optimization run, it's better to load the results from the h5 file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specifying initial rotations is likely needed for testing, so we need this. The docs can simply state that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'll 3rd the need to document explicit rotation via xml. Handling h5 can be cumbersome if one wants to do anything other than load in a previously optimized set of parameters. As others have pointed out, this is especially useful for testing, but should also be compatible with production calculations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rotations given via opt_vars
have limitations. They can't describe a full rotation - they only apply to the 'active' rotations (the ones with non-zero parameter derivatives). An arbitrary rotation either needs the full rotation matrix (global method) or a list of rotations (history list method), and those don't work with opt_vars.
Expanding opt_vars
or adding new tags to handle these cases would be possible. My preference would be the script route, though. It would help to have some more concrete ideas for production uses cases (e.g., preferred to have the original coefficient file + XML with global rotation to communicate an optimized wavefunction, versus a rotated coefficient file, versus the original coefficient file + HDF file with rotation information (how it works now))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait now I am confused. Isn't opt_vars specifying (some but not all) elements of the kappa matrix from which U=exp(-kappa)? Or are you pointing out that in general you can't completely specify an arbitrary transformation with the current <opt_vars> implementation? The latter is ok, imo. I just want to be able to poke orbitals without touching scripts or hdf5 files. I agree in production you'd have to rely on a more robust approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, opt_vars is a subset of the elements of the kappa matrix. And you need all the (strictly triangular) entries to specify an arbitrary rotation, which opt_vars does not provide. opt_vars is good for providing an initial rotation that moves the coefficient matrix away from the minimum in order to test the optimizer.
As an optional child for rotated_sposet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Test this please |
How does the user specify h5 inputs for orbital rotations? |
An HDF file with rotations can be specified with the |
In the spirit of keeping things moving, let merge this and discuss additional updates. |
Test this please |
Add documentation for orbital rotation.
What type(s) of changes does this code introduce?
Delete the items that do not apply
Does this introduce a breaking change?
What systems has this change been tested on?
laptop
Checklist
Update the following with a yes where the items apply. If you're unsure about any of them, don't hesitate to ask. This is
simply a reminder of what we are going to look for before merging your code.