-
Notifications
You must be signed in to change notification settings - Fork 135
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
Read and write orbital rotation parameters #4580
Conversation
Add functions for applying either global rotation or history list. The choice of history list vs. global rotation for a given run can be independent of how is was stored in the VP file. That is, the method used to restore the parameters in RotatedSPOs is determined by what method is stored in the VP file. Once the parameters in the RotatedSPO class are set up, it does not matter what method future optimization runs use.
hout.push("RotatedSPOs"); | ||
if (use_global_rot_) | ||
{ | ||
hid_t grp = hout.push("rotation_global"); |
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.
Do you mind picking up #4547?
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.
Does this need to be done in this PR though?
Merge this and keep moving?
@markdewing your preferences?
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 would prefer to make the change in #4547 separate from this PR.
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.
#4547 is definitely a separate PR. I would like it to be addressed instead of mentioning the same issue in multiple PRs. We can let this PR. go.
Test this please |
if (use_global_rot_) | ||
{ | ||
hid_t grp = hout.push("rotation_global"); | ||
std::string rot_global_name = std::string("rotation_global_") + SPOSet::getName(); |
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.
Could you add const if the variable won't be modified?
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.
We should have a separate discussion about what good ideas we want to enforce on the code. Adding 'const' to variables is not listed in the "Uses of const" section of the coding standards. And I question whether it's necessary for variables - it adds extra characters to the declaration without adding a lot of information (IMO).
src/QMCWaveFunctions/RotatedSPOs.cpp
Outdated
hid_t grp = hout.push("rotation_global"); | ||
std::string rot_global_name = std::string("rotation_global_") + SPOSet::getName(); | ||
|
||
int nparam = myVarsFull.size(); |
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.
Could you add const if the variable won't be modified?
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.
int nparam = myVarsFull.size();
seems to initialize nparam as myVarsFull.size()
and it may be modified.
const int nparam = myVarsFull.size();
made it clear. No modification will be applied on nparam
. It can be safely used as myVarsFull.size()
in the intended scope.
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 used nparam
as a shortcut name for myVarsFull.size()
here and as a shortcut name for myVars.size()
later. While these are technically in different scopes and the compiler is okay with the name reuse, it might be confusing to the reader. I updated the shortcut name to nparam_full
when referring to myVarsFull.size()
.
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.
That is exactly the purpose of using const to enhance readability. I have seen temp being reused in a similar way and completely destroys the readability.
else | ||
{ | ||
hid_t grp = hout.push("rotation_history"); | ||
size_t rows = history_params_.size(); |
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.
Could you add const if the variable won't be modified?
size_t rows = history_params_.size(); | ||
size_t cols = 0; | ||
if (rows > 0) | ||
cols = history_params_[0].size(); |
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.
const size_t cols = rows > 0 ? history_params_[0].size() : 0;
for (size_t j = 0; j < cols; j++) | ||
tmp(i, j) = history_params_[i][j]; | ||
|
||
std::string rot_hist_name = std::string("rotation_history_") + SPOSet::getName(); |
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.
Could you add const if the variable won't be modified?
if (!hin.getShape<RealType>(rot_param_name, sizes)) | ||
throw std::runtime_error("Failed to read rotation_params in VP file"); | ||
|
||
int nparam_actual = sizes[0]; |
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.
Could you add const if the variable won't be modified?
} | ||
|
||
hin.push("rotation_params", false); | ||
std::string rot_param_name = std::string("rotation_params_") + SPOSet::getName(); |
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.
Could you add const if the variable won't be modified?
throw std::runtime_error("Failed to read rotation_params in VP file"); | ||
|
||
int nparam_actual = sizes[0]; | ||
int nparam = myVars.size(); |
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.
Could you add const if the variable won't be modified?
@@ -21,6 +21,7 @@ | |||
#include "QMCWaveFunctions/EinsplineSetBuilder.h" | |||
#include "QMCWaveFunctions/RotatedSPOs.h" | |||
#include "checkMatrix.hpp" | |||
#include "FakeSPO.h" | |||
|
|||
#include <stdio.h> |
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.
stdio.h->cstdio
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.
This kind of change is starting to feel overly picky. Part of my motivation for #4559 was to make these kind of small but widespread updates to the code base separately. Then hopefully new code would pick up the changes by copying the updated code, and reviews would need to be less concerned about these kind of changes.
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.
Sorry about. Fair point. It is a line not part of this PR.
I actually just wanted to take a note as I was reading through the code base and raise awareness of developers.
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.
Although getting pushbacks, I still feel the need of using const more than the current state. Will update dev docs.
Test this please |
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.
As this gets tweaked further, I'll note that I do agree that adding const does improve readability, particularly for things like int nparam = myVars.size();
. A casual reader can more readily see what will or will not be changed.
Implement write/readVariationalParameters for orbital rotation.
The parameters associated with either global rotation or a history list are stored to the VP HDF file.
The parameters in myVars are also saved so the first call to resetParameters works correctly. The first call occurs in WaveFunctionFactory::buildTWF after the VP file is read. By "works correctly" , I mean the parameters set in myVars and the parameters in read into VariableSet match, so the change in parameters in RotatedSPOs::resetParametersExclusive is zero, and that causes no change to the carefully restored rotation matrices.
There are unit tests that do minimal testing of the code paths. There will need to be more complete tests of this functionality.
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?
desktop
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.