restructure hybrid model #1203

wangcj05 · 2020-04-07T17:24:30Z

Pull Request Description

What issue does this change request address? (Use "#" before the issue to link it, i.e., #42.)

see issue #1202

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.

1. Review all computer code.
2. If any changes occur to the input syntax, there must be an accompanying change to the user manual and xsd schema. If the input syntax change deprecates existing input files, a conversion script needs to be added (see Conversion Scripts).
3. Make sure the Python code and commenting standards are respected (camelBack, etc.) - See on the wiki for details.
4. Automated Tests should pass, including run_tests, pylint, manual building and xsd tests. If there are changes to Simulation.py or JobHandler.py the qsub tests must pass.
5. If significant functionality is added, there must be tests added to check this. Tests should cover all possible options. Multiple short tests are preferred over one large test. If new development on the internal JobHandler parallel system is performed, a cluster test must be added setting, in XML block, the node <internalParallel> to True.
6. If the change modifies or adds a requirement or a requirement based test case, the Change Control Board's Chair or designee also needs to approve the change. The requirements and the requirements test shall be in sync.
7. The merge request must reference an issue. If the issue is closed, the issue close checklist shall be done.
8. If an analytic test is changed/added is the the analytic documentation updated/added?
9. If any test used as a basis for documentation examples (currently found in raven/tests/framework/user_guide and raven/docs/workshop) have been changed, the associated documentation must be reviewed and assured the text matches the example.

wangcj05 · 2020-04-07T17:34:57Z

@alfoa @PaulTalbot-INL @mandd This is the first step for the restructuring. This PR is simply split the current HybridModel class into HybridModelBase and HybridModel classes. Please take a look, and more comments are favorable. My plan is first merge this PR to devel, and then address all comments in additional PR, since this PR just simply replicates our current functionalities.

PaulTalbot-INL · 2020-04-07T19:44:18Z

Just for clarification, HybridModelBase is the eventual name for the Logic and Hybrid model parent class?

PaulTalbot-INL

A couple initial comments, nothing fundamental.

framework/Models/HybridModels/Factory.py

PaulTalbot-INL · 2020-04-07T19:52:26Z

framework/Models/HybridModels/HybridModel.py

-      self.raiseAnError(IOError,'Model XML block needs to be inputted!')
+    if len(self.modelInstances) != 1:
+      self.raiseAnError(IOError, 'Required one "Model" XML subnode under node "HybridModel" can be accepted!',
+                        '"{}" "Model" subnodes are provided.'.format(len(self.modelInstances)))


sorry, I'm being annoying ... "HybridModel" node can only accept exactly one <Model> subnode! {n} subnodes were provided.

n subnodes is used in the base class. In HybridModel class, we only allow one model and multiple rooms, but in other model class, we can allow multiple models.

This was a grammar fix request; the original message is quite difficult to understand.

framework/Models/HybridModels/HybridModel.py

framework/Models/HybridModels/HybridModelBase.py

PaulTalbot-INL · 2020-04-07T19:58:32Z

framework/Models/HybridModels/HybridModelBase.py

+    """
+    for model in self.modelInstances:
+      if isinstance(model, Models.Model):
+        self.raiseAnError(IOError, "Model {} has already been initialized, and it can not be initialized again!".format(model.name))


This is interesting. I was under the impression each entity gets initialized once per Step that it's used in; does this mean a model used in a Step and then later as part of a Hybrid will error out?

This means if HybridModel is already initialized , it will error out, not specific model. Basically, I'm limiting the use of HybridModel in different steps. When it is moved forward to other models, i.e. LogicalModel, it will be more clear to me how to handle this. This is just a restructure with the same capabilities.

Is this valid for any model or just for the ROM? I think we added this in the Hybrid for the ROM to be sure to not perform a "destructive" process in case of already trained ROM. In here, is it going to error out for whatever model?

@alfoa You are right, it is used for the ROM. As I said, I prefer to change it in the next PR.

PaulTalbot-INL · 2020-04-07T19:58:51Z

framework/Models/HybridModels/HybridModelBase.py

+            codeInput.append(elem)
+        modelInstance.initialize(runInfo, codeInput, initDict)
+      self.modelInstances[model] = modelInstance
+  def getInitParams(self):


newline between methods

alfoa · 2020-04-13T14:49:44Z

framework/Models/HybridModels/Factory.py

+# limitations under the License.
+
+"""
+  Factory for generating the instances of the  HybridModels Module


do we really need a factory?
What do we buy with this?

alfoa · 2020-04-13T14:52:46Z

framework/Models/HybridModels/HybridModelBase.py

+      if isinstance(model, Models.Model):
+        self.raiseAnError(IOError, "Model {} has already been initialized, and it can not be initialized again!".format(model.name))
+      modelInstance = self.retrieveObjectFromAssemblerDict('Model', model)
+      if modelInstance.type == 'Code':


why do we need to be so specific?

Overall considering that in your "Logic Model" each mode can have a set of "different" input files

As discussed, will change in the next PR.

Do you think it is changeable in this PR? Just to be consistent.

@alfoa Personally, I would like to change it in the next PR.

PaulTalbot-INL · 2020-04-27T15:33:39Z

framework/Models/HybridModels/HybridModelBase.py

+    Dummy.__init__(self,runInfoDict)
+    self.modelInstances        = {}                  # dictionary {modelName: modelInstance}: instances of given model
+    self.sleepTime             = 0.005               # waiting time before checking if a run is finished.
+    self.printTag              = 'HybridModelBase MODEL' # print tag


This doesn't need changing, but note that in the MessageHandler the default trim length for tags is not very long (15 characters as I recall); this might be exceeding that length.

PaulTalbot-INL · 2020-04-27T15:36:51Z

I would really like to see that error message rewritten before merging; otherwise, no outstanding changes requested in the code.

moosebuild · 2020-04-27T17:06:20Z

Job Test Fedora 28 on 52f6dd0 : invalidated by @wangcj05

conda error when setting python environment

PaulTalbot-INL · 2020-04-27T18:09:19Z

it looks like we're getting an error only on Windows for the following:

(379/717) Diff   ( 15.55sec)tests\framework\PostProcessors\LimitSurface\testLimitSurfaceIntegralPPWihtBoundingError

@alfoa may have thoughts?

moosebuild · 2020-04-28T15:33:43Z

Job Mingw Test on 52f6dd0 : invalidated by @wangcj05

retest

moosebuild · 2020-05-06T15:17:06Z

Job Mingw Test on 52f6dd0 : invalidated by @wangcj05

wangcj05 · 2020-05-06T16:22:12Z

@PaulTalbot-INL @alfoa Tests are green for this PR. Could you merge this?

PaulTalbot-INL · 2020-05-06T17:07:30Z

Checklists have passed, but merging is blocked by @alfoa change requests.

alfoa · 2020-05-06T17:10:38Z

Approved. Please @wangcj05 open an issue regarding the outstanding changes that you want to do in a follow-up PR.

wangcj05 added 2 commits April 6, 2020 10:53

initial restructure of HybridModel

0d8c987

update modelInstances to accept multiple model entities

fc3967a

wangcj05 requested review from alfoa, mandd and PaulTalbot-INL April 7, 2020 17:28

PaulTalbot-INL reviewed Apr 7, 2020

View reviewed changes

alfoa requested changes Apr 13, 2020

View reviewed changes

wangcj05 added 3 commits April 15, 2020 12:05

Merge branch 'devel' into wangc/hybrid_rework

a6effa6

remove factory

cf4549d

address comments

7daadb0

PaulTalbot-INL reviewed Apr 27, 2020

View reviewed changes

wangcj05 added 2 commits April 27, 2020 10:42

Merge branch 'devel' into wangc/hybrid_rework

148b264

update error message

52f6dd0

PaulTalbot-INL approved these changes May 6, 2020

View reviewed changes

PaulTalbot-INL requested a review from alfoa May 6, 2020 17:06

alfoa approved these changes May 6, 2020

View reviewed changes

PaulTalbot-INL merged commit 381c013 into devel May 6, 2020

alfoa deleted the wangc/hybrid_rework branch May 6, 2020 21:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

restructure hybrid model #1203

restructure hybrid model #1203

wangcj05 commented Apr 7, 2020 •

edited by PaulTalbot-INL

Loading

wangcj05 commented Apr 7, 2020

PaulTalbot-INL commented Apr 7, 2020

PaulTalbot-INL left a comment

PaulTalbot-INL Apr 7, 2020

wangcj05 Apr 15, 2020

PaulTalbot-INL Apr 27, 2020

wangcj05 Apr 27, 2020

PaulTalbot-INL Apr 7, 2020

wangcj05 Apr 15, 2020

alfoa Apr 20, 2020

wangcj05 Apr 21, 2020

PaulTalbot-INL Apr 7, 2020

wangcj05 Apr 15, 2020

alfoa Apr 13, 2020

wangcj05 Apr 15, 2020

alfoa Apr 13, 2020

wangcj05 Apr 15, 2020

alfoa Apr 20, 2020

wangcj05 Apr 21, 2020

PaulTalbot-INL Apr 27, 2020

PaulTalbot-INL commented Apr 27, 2020

moosebuild commented Apr 27, 2020

PaulTalbot-INL commented Apr 27, 2020

moosebuild commented Apr 28, 2020

moosebuild commented May 6, 2020

wangcj05 commented May 6, 2020

PaulTalbot-INL commented May 6, 2020

alfoa commented May 6, 2020

restructure hybrid model #1203

restructure hybrid model #1203

Conversation

wangcj05 commented Apr 7, 2020 • edited by PaulTalbot-INL Loading

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?

For Change Control Board: Change Request Review

wangcj05 commented Apr 7, 2020

PaulTalbot-INL commented Apr 7, 2020

PaulTalbot-INL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PaulTalbot-INL commented Apr 27, 2020

moosebuild commented Apr 27, 2020

PaulTalbot-INL commented Apr 27, 2020

moosebuild commented Apr 28, 2020

moosebuild commented May 6, 2020

wangcj05 commented May 6, 2020

PaulTalbot-INL commented May 6, 2020

alfoa commented May 6, 2020

wangcj05 commented Apr 7, 2020 •

edited by PaulTalbot-INL

Loading