-
Notifications
You must be signed in to change notification settings - Fork 144
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
Merged
Merged
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
33be2d3
Add oribital rotation documentation
markdewing 30f8cc2
Fix attribute on rotated_sposet
markdewing 5554e51
Document opt_vars element
markdewing a6cef56
Merge branch 'develop' into orb_rot_doc
ye-luo fcd15cd
Tweak description
prckent 26abc43
Merge branch 'develop' into orb_rot_doc
ye-luo File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.