-
Notifications
You must be signed in to change notification settings - Fork 136
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
Extend Restart to Adaptive samplers #1283
Conversation
Test invalidated to get the new modifications in the BasicStats tests (see PR #1282) |
Job Mingw Test on 3c1f0e2 : 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.
some minor comments
""" | ||
#for future compatibility with Python 3-------------------------------------------------------------- | ||
from __future__ import division, print_function, unicode_literals, absolute_import | ||
#End compatibility block for Python 3---------------------------------------------------------------- |
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 think we do not need this import anymore.
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 believe the linter will error if it doesn't find this. We should change this globally, as a separate action.
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.
@alfoa I think you are trying to remove the __future__
import, 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.
I think we removed the check on that because it is not needed anymore... @joshua-cogliati-inl ???
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 we removed it. we can remove the "future" (I know we need to do that to the other files too...I will take care of it)
handler object | ||
@ In, args, dict, this is a list of arguments that will be passed as | ||
function parameters into whatever method is stored in functionToRun. | ||
e.g., functionToRun(*args) |
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.
change args to data
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.
done
<description> | ||
Tests restarting an Adaptive Monte Carlo sampling from restart. \texttt{makeCoarse} samples initial data, then \texttt{makeRestart} | ||
makes additional samples, restarting from the first set of samples. \texttt{makeFine} does all the samples without restart | ||
for comparison. The model for "coarse" always returns a value of 1, while the model for "restart" returns a value of 2, so |
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.
use ``'' instead ""?
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.
done, used texttt instead
<?xml version="1.0" ?> | ||
<Simulation verbosity="debug"> | ||
<TestInfo> | ||
<name>framework/Samplers/Restart.AdaptiveMC</name> |
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.
change AdaptiveMC to AMC
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.
done
@alfoa Changes are good to me. Do you want to have review on this PR? |
""" | ||
#for future compatibility with Python 3-------------------------------------------------------------- | ||
from __future__ import division, print_function, unicode_literals, absolute_import | ||
#End compatibility block for Python 3---------------------------------------------------------------- |
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 we removed it. we can remove the "future" (I know we need to do that to the other files too...I will take care of it)
Comments addressed... Checklist passed... PR can be merged |
Pull Request Description
What issue does this change request address? (Use "#" before the issue to link it, i.e., #42.)
Closes #1280
What are the significant changes in functionality due to this change request?
Extends expected existing restart mechanics to Adaptive samplers. This can be improved once:
generateInput
returns a realization instead of storing onself
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.