Ensemble Model Connections Improvement #732

wangcj05 · 2018-08-04T20:51:58Z

Pull Request Description

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

close #696

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?

…models

alfoa · 2018-08-04T22:25:17Z

I already reviewed a PR like this... where did it end up?

alfoa · 2018-08-05T17:09:30Z

framework/utils/graphStructure.py

+    graphDict = copy.deepcopy(self.__graphDict)
+    # Extend graph dictionary generated by ensemble model to graph theory acceptable dictionary
+    # Basicly, if a -> b (a is connected to b), the self.__graphDict = {'a':['b'], 'b':[]}
+    # Since a is connected to b, from the graph theory, the dictionary should become {'a':['b'], 'b':['a']}


This class was supposed to represent a directed graph since was mainly used by the ensemble model for the identification of the execution order.
Please, put a WARNING to avoid to store this extended graph dictionary in __graphDict and specify that this should be used for the isConnected method only.

In addition, enhance the comment above (and in the above docstrings) to state this.

alfoa · 2018-08-05T17:10:20Z

tests/framework/ensembleModelTests/checkConnection/alpha.py

+# NEEDS TO BE THREAD-SAFE!!!!!
+# IN CASE IT IS NOT, THE USER NEEDS TO SET A LOCK SYSTEM!!!!!
+import threading
+localLock = threading.RLock()


you do not need the lock here (for this model). Please remove it and remove the comment above. In addition all the above imports are not used.

alfoa · 2018-08-05T17:10:41Z

tests/framework/ensembleModelTests/checkConnection/alpha.py

+localLock = threading.RLock()
+
+def initialize(self, runInfo, inputs):
+  pass


it is not needed... if it is not present, it will not be called.

alfoa · 2018-08-05T17:11:07Z

tests/framework/ensembleModelTests/checkConnection/alpha.py

+  pass
+
+def run(self, Input):
+  self.C = self.A + self.B


add docstrings to describe what this model is used for

alfoa · 2018-08-05T17:11:30Z

tests/framework/ensembleModelTests/checkConnection/beta.py

+localLock = threading.RLock()
+
+def initialize(self, runInfo, inputs):
+  pass


same comments I reported for model alpha.py

alfoa · 2018-08-05T17:11:53Z

tests/framework/ensembleModelTests/checkConnection/delta.py

+  pass
+
+def run(self, Input):
+  self.F = self.E + 1.0


same comments i reported for the other 2 models

alfoa · 2018-08-05T17:11:59Z

tests/framework/ensembleModelTests/checkConnection/epsilon.py

+  pass
+
+def run(self, Input):
+  self.G = self.H + 3.0


alfoa · 2018-08-05T17:12:07Z

tests/framework/ensembleModelTests/checkConnection/gamma.py

+  pass
+
+def run(self, Input):
+  self.D = self.G + 1.0


alfoa · 2018-08-05T17:12:13Z

tests/framework/ensembleModelTests/checkConnection/rho.py

+  pass
+
+def run(self, Input):
+  self.I = self.D - 1.0


alfoa · 2018-08-05T17:14:36Z

tests/framework/ensembleModelTests/tests

+ [./testEMConnection]
+   type = 'RavenFramework'
+   input = 'test_em_connection.xml'
+   output = 'checkConnection/1-plotData_scatter.png'


either you check the plot with an Image Checker or you dump a csv for comparing with a gold file.

Second option is the preferred one (in case, remove the plot that it will be useless)

moosebuild · 2018-08-08T14:50:27Z

Job Mingw Test on be29bcb : invalidated by @wangcj05

test time out

wangcj05 · 2018-08-09T14:45:12Z

@alfoa This is ready for you to review.

alfoa

Comments addressed...

alfoa · 2018-08-09T14:52:02Z

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. done
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). done
3. Make sure the Python code and commenting standards are respected (camelBack, etc.) - See on the wiki for details. done
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. they 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. ok
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. n/a
7. The merge request must reference an issue. If the issue is closed, the issue close checklist shall be done. The connected net check in the ensemble model can return false even if the model are connected #696
8. If an analytic test is changed/added is the the analytic documentation updated/added? n/a

alfoa · 2018-08-09T14:52:14Z

Checklist passed...

Merging...

wangcj05 added 2 commits August 4, 2018 13:40

fix the graph theory used to check the connections between different …

7278b94

…models

add test

ffed4c7

wangcj05 requested a review from alfoa August 4, 2018 20:51

alfoa requested changes Aug 5, 2018

View reviewed changes

resolve comments, update tests and docstring

be29bcb

alfoa approved these changes Aug 9, 2018

View reviewed changes

alfoa merged commit 02ba132 into idaholab:devel Aug 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensemble Model Connections Improvement #732

Ensemble Model Connections Improvement #732

wangcj05 commented Aug 4, 2018 •

edited by alfoa

Loading

alfoa commented Aug 4, 2018

alfoa Aug 5, 2018

wangcj05 Aug 6, 2018

alfoa Aug 5, 2018

wangcj05 Aug 6, 2018

alfoa Aug 5, 2018

wangcj05 Aug 6, 2018

alfoa Aug 5, 2018

wangcj05 Aug 6, 2018

alfoa Aug 5, 2018

wangcj05 Aug 6, 2018

alfoa Aug 5, 2018

wangcj05 Aug 6, 2018 •

edited

Loading

alfoa Aug 5, 2018

wangcj05 Aug 6, 2018

alfoa Aug 5, 2018

wangcj05 Aug 6, 2018

alfoa Aug 5, 2018

wangcj05 Aug 6, 2018

alfoa Aug 5, 2018

wangcj05 Aug 6, 2018

moosebuild commented Aug 8, 2018

wangcj05 commented Aug 9, 2018

alfoa left a comment

alfoa commented Aug 9, 2018

alfoa commented Aug 9, 2018

Ensemble Model Connections Improvement #732

Ensemble Model Connections Improvement #732

Conversation

wangcj05 commented Aug 4, 2018 • edited by alfoa 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

alfoa commented Aug 4, 2018

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

wangcj05 Aug 6, 2018 • edited Loading

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

moosebuild commented Aug 8, 2018

wangcj05 commented Aug 9, 2018

alfoa left a comment

Choose a reason for hiding this comment

alfoa commented Aug 9, 2018

For Change Control Board: Change Request Review

alfoa commented Aug 9, 2018

wangcj05 commented Aug 4, 2018 •

edited by alfoa

Loading

wangcj05 Aug 6, 2018 •

edited

Loading