-
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
Closes #1493 - Fix Conjugated Gradient #1494
Conversation
Job Mingw Test on 51dcdfa : invalidated by @alfoa |
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.
Code changes are fine; I am not familiar enough with CG to say whether it is now working as it should be.
Job Mingw Test on 51dcdfa : invalidated by @joshua-cogliati-inl restarted civet |
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.
Approved
I'm seeing the following test failures:
These both seem to be common to multiple PRs right now. The difference in SyntheticHistory.ARMA is in a relatively small (but not quite insignificant) value, so perhaps the tolerance could be increased, but I hate doing that without knowing what libraries changed ... |
I think CentOS 7 is fixed. Our OpenSuse Leap system is having some weird DNS issue that neither Derek nor I have had time to try and figure out and fix. |
Job Test CentOS 7 on 51dcdfa : invalidated by @joshua-cogliati-inl restarted civet |
Job Test qsubs on 12746c8 : invalidated by @alfoa ? |
Job Test CentOS 7 on e22815c : invalidated by @joshua-cogliati-inl Failed with scripts/establish_conda_env.sh: line 136: /home/civet/civet/build_0/raven/plugins/plugin_directory.xml: Permission denied |
@@ -381,7 +381,7 @@ def _startLineSearch(self, lastStepInfo, curPoint, curObjVal, curGrad, curGradMa | |||
# since we've accepted a pivot, we need to store the old pivot and set up the new one | |||
## first grab the savable params | |||
pivot = lastStepInfo.pop('pivot', None) | |||
if True: #pivot is None: | |||
if pivot is None: |
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 is the only mechanical change in this PR, right?
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
@aalfonsi Could you address the commends in this week so that we can include these changes in new release? |
addressed @wangcj05 @PaulTalbot-INL |
Pull Request Description
What issue does this change request address? (Use "#" before the issue to link it, i.e., #42.)
Closes #1493
What are the significant changes in functionality due to this change request?
A debug statement was still in the code. This was not allowing the line-search to be activated.
The Conjugated Gradient basically was not doing what the method was supposed to do.
For Change Control Board: Change Request Review
The following review must be completed by an authorized member of the Change Control Board.
<internalParallel>
to True.raven/tests/framework/user_guide
andraven/docs/workshop
) have been changed, the associated documentation must be reviewed and assured the text matches the example.