-
Notifications
You must be signed in to change notification settings - Fork 133
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
Ensemble model fixes for batching #2007
Conversation
Job Test qsubs sawtooth on ad894b9 : invalidated by @wangcj05 one failed run for tests/cluster_tests/InternalParallel/test_hybrid_model_code |
Job Test qsubs sawtooth on ad894b9 : invalidated by @joshua-cogliati-inl fixed problem on sawtooth |
Job Mingw Test on ad894b9 : invalidated by @wangcj05 retesting |
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.
@mandd I have some comments for you to consider.
if output.name not in optionalOutputNames: | ||
if output.name not in targetEvaluationNames.keys(): | ||
output.addRealization(joinedResponse) | ||
if isinstance(evaluation,dict): |
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 update this line with followings:
# in the event a batch is run, the evaluations will be a dict as {'RAVEN_isBatch':True, 'realizations': [...]}
if isinstance(evaluation,dict) and evaluation.get('RAVEN_isBatch',False):
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.
added
metadata = kw | ||
|
||
if self.parallelStrategy == 1: | ||
jobHandler.addJob((self, myInput, samplerType, kw), self.__class__.evaluateSample, prefix, metadata=metadata, |
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.
In addition, we need add a test for this PR to test the GA with ensemble model
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.
test added
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.
changes look good.
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 missing "classesTested" node in TestInfo causes the XSD schema checks failed. Could you update that? @mandd
<?xml version="1.0" ?> | ||
<Simulation verbosity="debug"> | ||
<TestInfo> | ||
<name>framework/Optimizers/GeneticAlgorithms/test_ensemble_withGA</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.
classesTested node is required. Could you add 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.
fixed!
PR looks good and checklist is satisfied. |
Pull Request Description
What issue does this change request address? (Use "#" before the issue to link it, i.e., #42.)
What are the significant changes in functionality due to this change request?
close #2019
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.