-
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
Alfoa/hybrid model for batching and ensemble model #2322
base: devel
Are you sure you want to change the base?
Conversation
…h those since I am here)
…github.com:idaholab/raven into alfoa/hybrid_model_for_batching_and_ensemble_model
@@ -136,7 +136,7 @@ def localInputAndChecks(self,xmlNode): | |||
self.raiseAnError(IOError, "Input XML node for Model" + modelName +" has not been inputted!") | |||
if len(self.modelsInputDictionary[modelName].values()) > allowedEntriesLen: | |||
self.raiseAnError(IOError, "TargetEvaluation, Input and metadataToTransfer XML blocks are the only XML sub-blocks allowed!") | |||
if child.attrib['type'].strip() == "Code": | |||
if child.attrib['type'].strip() in ["Code", 'HybridModel', 'LogicalModel']: |
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.
very ugly :(
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 would be slightly less ugly if you used a set:
if child.attrib['type'].strip() in {"Code", 'HybridModel', 'LogicalModel'}:
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
@@ -267,6 +268,12 @@ def initialize(self,runInfo,inputs,initDict=None): | |||
|
|||
# initialize model | |||
self.modelsDictionary[modelName]['Instance'].initialize(runInfo,inputInstancesForModel,initDict) | |||
if modelType in ['HybridModel', 'LogicalModel']: |
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 should be replaced by issubclass(self.modelsDictionary[modelName]['Instance'], HybridModelBase)
?
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 suggestion will be better 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.
done
@@ -136,7 +136,7 @@ def localInputAndChecks(self,xmlNode): | |||
self.raiseAnError(IOError, "Input XML node for Model" + modelName +" has not been inputted!") | |||
if len(self.modelsInputDictionary[modelName].values()) > allowedEntriesLen: | |||
self.raiseAnError(IOError, "TargetEvaluation, Input and metadataToTransfer XML blocks are the only XML sub-blocks allowed!") | |||
if child.attrib['type'].strip() == "Code": | |||
if child.attrib['type'].strip() in ["Code", 'HybridModel', 'LogicalModel']: | |||
self.createWorkingDir = True |
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 is ugly and in addition this creates a sub-working directory even if, for example, the Logical/hybrid models do not use a Code
. In case of HybridModel/LogicalModel using only ExternalModels/ROMs, the subdirectory is created but will stay empty. Not very elegant. @joshua-cogliati-inl @wangcj05 any ideas on how to improve this?
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.
@wangcj05 @joshua-cogliati-inl any ideas for this? I cannot find a better solution at this stage
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 we have a class attribute to indicate if there is a code associated with the Model? For example, In Hybrid Model/Logical/Ensemble Model, we define a self._isCodeAvail
, and assign it to true when we detect a code in the Model. @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.
Hm, I am trying to fully understand why you need to create the directory? Just to check, it is needed if the Logical/hybrid models use a Code? (Congjian's idea sounds reasonable)
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 that's why. If there is a Code in the underlying Logical/Hybrid model (contained in the ensemble model) the subfolder is required.
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.
@wangcj05 @joshua-cogliati-inl can you tell me how exactly you would like that flag to be coded? (I prefer not to take a code design decision (that might be needed to be modified) on my own)?
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.
@wangcj05 @joshua-cogliati-inl FY: if you can send feedbacks within tomorrow I can try to address them before leaving on Friday. Otherwise it will need to wait till September.
tests/framework/hybridModel/test_logical_ensemble_model_and_code.xml
Outdated
Show resolved
Hide resolved
tests/framework/hybridModel/test_logical_ensemble_model_and_code.xml
Outdated
Show resolved
Hide resolved
Job Mingw Test on ab91508 : invalidated by @joshua-cogliati-inl restarted civet |
…rashes if there are variables that are not called to be optimized but are strings
Job Mingw Test on b25a176 : invalidated by @joshua-cogliati-inl failed scripts/establish_conda_env.sh: line 153: 21634 Segmentation fault |
…github.com:idaholab/raven into alfoa/hybrid_model_for_batching_and_ensemble_model
# start the job handler | ||
self.localPollingThread = InterruptibleThread(target=self.localJobHandler.startLoop) | ||
self.localPollingThread.daemon = True | ||
self.localPollingThread.start() | ||
# register function to kill the thread at the end of the execution | ||
atexit.register(self.localPollingThread.kill) |
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.
Hm, pollingThread (both here and in Simulation.py) possibly should be inside JobHandler. That said, this is just mimicking how Simulation does this, so I am not sure if this needs to be fixed now.
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.
Also, can the localPollingThread be killed sooner, rather than waiting for the program to exit?
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 Could you address Josh's comment 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.
added in a dedicated function that is executed at the end of the step
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 have some general comments for you to consider.
nPoints = preVolumeCalc.attrib.get("nPoints") | ||
if nPoints is not None: | ||
self.nVolumePoints = utils.intConversion(utils.floatConversion(nPoints)) | ||
print(nPoints) |
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.
removed
@@ -267,6 +268,12 @@ def initialize(self,runInfo,inputs,initDict=None): | |||
|
|||
# initialize model | |||
self.modelsDictionary[modelName]['Instance'].initialize(runInfo,inputInstancesForModel,initDict) | |||
if modelType in ['HybridModel', 'LogicalModel']: |
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 suggestion will be better here.
# start the job handler | ||
self.localPollingThread = InterruptibleThread(target=self.localJobHandler.startLoop) | ||
self.localPollingThread.daemon = True | ||
self.localPollingThread.start() | ||
# register function to kill the thread at the end of the execution | ||
atexit.register(self.localPollingThread.kill) |
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 Could you address Josh's comment here.
if not isThereACode: | ||
isThereACode = modelType == 'Code' | ||
|
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 would suggest we add these lines in Logical model and Hybrid 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.
I addressed this as well.
@wangcj05 @joshua-cogliati-inl I addressed all the comments. |
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.
@joshua-cogliati-inl Do you have other comments?
Pull Request Description
What issue does this change request address? (Use "#" before the issue to link it, i.e., #42.)
Closes #2321
What are the significant changes in functionality due to this change request?
The development here is addressing 2 issues:
batching
strategy (e.g. GeneticAlgorithm).In addition, an utility factory has been added in the JobHandler to collect and record Job Ids that have been used during the simulation. For now it is used as error checker but in the future can be used as base-class for identifier creation.
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.