-
Notifications
You must be signed in to change notification settings - Fork 137
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
Mandd/gawithcustomsampler #2084
Conversation
Thanks a lot! |
Job Test Fedora 31 on 36d81f6 : invalidated by @mandd |
Job Test Ubuntu 18-2 Python 3 on 36d81f6 : invalidated by @mandd |
1 similar comment
Job Test Ubuntu 18-2 Python 3 on 36d81f6 : invalidated by @mandd |
@@ -106,7 +107,9 @@ def getpotToInputTree(getpot): | |||
#------------------ | |||
# attributes and values | |||
elif '=' in line: | |||
print('HERE') |
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.
remove?
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, edit for a different issue
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 have a coupe of minor comments for you to consider. @mandd
attribute, value = (x.strip() for x in line.split('=', maxsplit=1)) | ||
print(attribute, value) |
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.
remove?
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, edit for a different issue
@@ -124,6 +127,7 @@ def getpotToInputTree(getpot): | |||
# DO NOT continue, keep going until multiline is closed | |||
continue | |||
else: | |||
print('HERE1') |
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.
remove?
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, edit for a different issue
@@ -208,7 +208,8 @@ def _localGenerateAssembler(self, initDict): | |||
self.assemblerDict['DataObjects'] = [] | |||
self.assemblerDict['Distributions'] = [] | |||
self.assemblerDict['Functions'] = [] | |||
for mainClass in ['DataObjects', 'Distributions', 'Functions']: | |||
self.assemblerDict['Files'] = [] | |||
for mainClass in ['DataObjects', 'Distributions', 'Functions', 'Files']: |
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.
maybe define a validAssembler list which can be used in multiple places.
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
<objective>ans</objective> | ||
<TargetEvaluation class="DataObjects" type="PointSet">optOut</TargetEvaluation> | ||
<Sampler class="Samplers" type="CustomSampler">restartSampler</Sampler> | ||
<!--Sampler class="Samplers" type="MonteCarlo">MC_samp</Sampler--> |
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.
remove?
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
<MonteCarlo name="MC_samp"> | ||
<samplerInit> | ||
<limit>10</limit> | ||
<initialSeed>20021986</initialSeed> | ||
</samplerInit> | ||
<variable name="x1"> | ||
<distribution>uniform_dist_Repl_1</distribution> | ||
</variable> | ||
<variable name="x2"> | ||
<distribution>uniform_dist_Repl_1</distribution> | ||
</variable> | ||
<variable name="x3"> | ||
<distribution>uniform_dist_Repl_1</distribution> | ||
</variable> | ||
</MonteCarlo> |
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.
remove? Not used.
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
Job Test CentOS 7 on 0473401 : invalidated by @mandd |
@@ -401,7 +405,7 @@ def _initializeInitSampler(self, externalSeeding): | |||
self._initSampler = sampler | |||
# initialize sampler | |||
samplerInit = {} | |||
for entity in ['Distributions', 'Functions', 'DataObjects']: | |||
for entity in ['Distributions', 'Functions', 'DataObjects','Files']: |
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 change the list to use self.optAssemblerList here?
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.
good catch
needDict['Distributions'] = [(None,'all')] | ||
needDict['Functions' ] = [(None,'all')] | ||
needDict['DataObjects' ] = [(None,'all')] | ||
needDict['Files' ] = [(None,'all')] |
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.
It seems we can do a loop over self.optAssemblerList here.
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.
good point
Job Test Ubuntu 16 on 0040e2a : invalidated by @mandd |
#needDict['Distributions'] = [(None,'all')] | ||
#needDict['Functions' ] = [(None,'all')] | ||
#needDict['DataObjects' ] = [(None,'all')] | ||
#needDict['Files' ] = [(None,'all')] |
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.
remove these lines?
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
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.
doing too many things at the same time
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.
Once tests are green, this PR can be merged. |
Job Mingw Test on 7a31828 : invalidated by @wangcj05 fetch failed |
Job Mingw Test on 7a31828 : invalidated by @joshua-cogliati-inl failed in fetch |
Job Mingw Test on 7a31828 : invalidated by @mandd |
Job Mingw Test on 7a31828 : invalidated by @wangcj05 failed InterpolatedMaxCycles |
Job Mingw Test on 7a31828 : invalidated by @mandd |
@mandd I guess there is a consistent failure in Windows, which is caused by test InterpolatedMaxCycles. This test has been recently revived in PR#2035. The purpose of the test is test the validity of input structure for ARMA, it is not made for consistent check. I recommend to increase the rel_err for this test or we do not check the consistency in the csv file, just checkout it produces csv files. |
I agree, change "csv =" to "output =" in this test. |
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.)
Closes #2060
What are the significant changes in functionality due to this change request?
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.