-
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
Multiple Inputs for dummy models #2006
Conversation
Job Mingw Test on 8a75a8e : canceled by @joshua-cogliati-inl needs to restart |
Job Mingw Test on 8a75a8e : invalidated by @joshua-cogliati-inl civet was slow |
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 a comment for you to review.
if len(myInput)>1: | ||
self.raiseAnError(IOError,'Only one input is accepted by the model type '+self.type+' with name'+self.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.
@mandd I guess we can remove this constraint for external model, but I think we still need it for ROM. Could you just remove this constraint in the external 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.
Fixed
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 some comments for you to consider.
ravenframework/Models/ROM.py
Outdated
def _manipulateInput(self,dataIn): | ||
""" | ||
Method that is aimed to manipulate the input in order to return a common input understandable by this class | ||
@ In, dataIn, object, the object that needs to be manipulated | ||
@ Out, inRun, dict, the manipulated input | ||
""" | ||
if len(dataIn)>1: | ||
self.raiseAnError(IOError,'Only one input is accepted by the model type '+self.type+' with name'+self.name) | ||
inRun = super()._manipulateInput(dataIn) | ||
return inRun |
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 you can remove these lines. I think we only need to keep one check
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
ravenframework/Models/ROM.py
Outdated
[(inputDict)],kwargs = super().createNewInput(myInput,samplerType,**kwargs) | ||
return [(inputDict)],copy.deepcopy(kwargs) |
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 a little bit redundant here, may be you can do:
newInput, kwargs = super().createNewInput(myInput,samplerType,**kwargs)
return newInput, kwargs
I think there is no need to do deep copy here, since the deep copy is handled in super class.
If you change the code, please also update the docstring.
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.
deepcopy removed, i kept the original format for consistency
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.
One additional comment @mandd
ravenframework/Models/ROM.py
Outdated
@@ -172,6 +172,35 @@ def applyRunInfo(self, runInfo): | |||
""" | |||
self.numThreads = runInfo.get('NumThreads', 1) | |||
|
|||
def _manipulateInput(self,dataIn): |
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.
Can you remove this method from the ROM class? I think it is not needed. @mandd
The check has been performed in createNewInput
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, removed
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.
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.)
close #2005
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.