Basic Event Scheduler Model #37

mandd · 2021-07-15T16:07:56Z

Pull Request Description

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

Closes #36

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.
3. Make sure the Python code and commenting standards are respected (camelBack, etc.) - See on the wiki for details.
4. Regression tests have to complete successfully.
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.
6. The merge request must reference an issue. If the issue is closed, the issue close checklist shall be done.

wangcj05

I have couple of comments for your consideration.

wangcj05 · 2021-07-20T20:29:26Z

__init__.py

@@ -9,6 +9,7 @@
 from SR2ML.src import MaintenanceModel
 from SR2ML.src import MCSSolver
 from SR2ML.src import MarginModel
+from SR2ML.src import basicEventScheduler


I would recommend to use upper case for each leading character when define a class. That is: change basicEventScheduler to BasicEventScheduler

wangcj05 · 2021-07-20T20:30:50Z

doc/user_manual/include/basicEventScheduler.tex

+  \item  \xmlNode{BE}, \xmlDesc{string, required parameter}, the name ID of all basic events
+	  \begin{itemize}
+	    \item \xmlAttr{tin}, \xmlDesc{required string attribute}, name ID of the variable describing the initial time of the BE
+	    \item \xmlAttr{tin}, \xmlDesc{required string attribute}, name ID of the variable describing the final time of the BE


change 'tin' to 'tfin'

fixed, good catch

wangcj05 · 2021-07-20T20:33:54Z

src/basicEventScheduler.py

+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.


please update the copyright info, (The above info is used by raven). Something like:

# Copyright 2020, Battelle Energy Alliance, LLC # ALL RIGHTS RESERVED

wangcj05 · 2021-07-20T20:36:19Z

tests/test_basicEventScheduler.xml

@@ -0,0 +1,119 @@
+<Simulation verbosity="debug">


no gold files for this test

That's weird, i wonder why the regression test was green

see issue #38

wangcj05 · 2021-07-20T20:36:25Z

tests/tests

+ [./TestBasicEventScheduler]
+  type  = 'RavenFramework'
+  input = 'test_basicEventScheduler.xml'
+  OrderedCsv   = 'basicEventScheduler/Print_sim_HS_0.csv'


the csv file is missed in the gold folder

change OrderedCsv to csv, otherwise, the test will be be tested.

wangcj05 · 2021-07-20T20:36:54Z

src/basicEventScheduler.py

+    """
+      This method generate an historySet from the a pointSet which contains initial and final time of the
+      basic events
+      @ In, inputDataset, dict, dictionary of inputs from RAVEN


change inputDataset to inputs

wangcj05 · 2021-07-20T20:39:13Z

src/basicEventScheduler.py

+      basic events
+      @ In, inputDataset, dict, dictionary of inputs from RAVEN
+      @ In, container, object, self-like object where all the variables can be stored
+      @ Out, basicEventHistorySet, Dataset, xarray dataset which contains time series for each basic event


there is no return for the run method. I think you can move the above line to the method docstring explanation part.

wangcj05 · 2021-07-20T20:43:14Z

src/basicEventScheduler.py

+      elif child.tag == 'variables':
+        variables = [str(var.strip()) for var in child.text.split(",")]


The variables is not directly used in this class, you probably can remove these lines.

The lines are needed as part of the external model class which require the "variables" node

The variables are processed in ExternalModel class, but you are not using it in your class.

I removed lines 65 and 66 but it errors out of line 68

You are right. We are looping over all nodes, even if they have already processed by other class. I'm ok to keep it here.

wangcj05 · 2021-07-20T20:53:14Z

doc/user_manual/include/basicEventScheduler.tex

+
+This model is designed to read the initial and final time under which a set of basic events
+are set to True, and then it generates an HistorySet with the full temporal profile of all
+basic events.


I recommend to add another paragraph to explain the structure of generated history set.

wangcj05 · 2021-07-20T20:54:38Z

src/basicEventScheduler.py

+    for key in container.basicEvents.keys():
+      tin  = inputs[container.basicEvents[key][0]][0]
+      tend = inputs[container.basicEvents[key][1]][0]
+      indexes = np.where(np.logical_and(timeArrayCleaned>tin,timeArrayCleaned<=tend))


why 'timeArrayCleaned>tin', but not 'timeArrayCleaned>=tin'?

This is due to to the conversion from interval-based data to instant-based data (i.e., the history set). The convention here is that a logical value set to 1 at a specific time instant implies that the basic event was actually True from the previous time instant

I see your point, it is better to clarify it in the user manual.

Agree, added text.

Based on the definition, 'tin' is the initial time for the basic event. Based on the calculation np.where(np.logical_and(timeArrayCleaned>tin,timeArrayCleaned<=tend)), when the time instant is tin, the value are set to 0. Does this mean the event are still the same as before at tin, and does not change state at tin? Only the time large than tin, the basic event are actually changing state. Do I state correctly? @mandd

mandd · 2021-07-26T19:09:25Z

@wangcj05 : let me know if you need more edit form me here

wangcj05

This PR is good. @mandd Could you resolve the conflict in the tests file?

mandd added 4 commits July 14, 2021 17:17

added src and test file

6fab7d1

added documentation

53b7eaf

delete whitepsaces

512eaf6

cleaning code

27dcbba

mandd requested a review from wangcj05 July 15, 2021 16:27

mandd added priority normal task labels Jul 15, 2021

wangcj05 requested changes Jul 20, 2021

View reviewed changes

mandd added 3 commits July 20, 2021 17:12

first review

571c36c

added text in manual

7eb7c07

forgot last edit

a5cb574

wangcj05 requested changes May 10, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic Event Scheduler Model #37

Basic Event Scheduler Model #37

mandd commented Jul 15, 2021

wangcj05 left a comment

wangcj05 Jul 20, 2021

mandd Jul 20, 2021

wangcj05 Jul 20, 2021

mandd Jul 20, 2021

wangcj05 Jul 20, 2021

mandd Jul 20, 2021

wangcj05 Jul 20, 2021

mandd Jul 20, 2021

mandd Jul 20, 2021

wangcj05 Jul 20, 2021

mandd Jul 20, 2021

wangcj05 Jul 20, 2021

mandd Jul 20, 2021

wangcj05 Jul 20, 2021

mandd Jul 20, 2021

wangcj05 Jul 20, 2021

mandd Jul 20, 2021

wangcj05 Jul 20, 2021

mandd Jul 20, 2021

wangcj05 Jul 20, 2021

mandd Jul 20, 2021

wangcj05 Jul 21, 2021

wangcj05 Jul 20, 2021

mandd Jul 20, 2021

wangcj05 Jul 20, 2021

mandd Jul 20, 2021

wangcj05 Jul 21, 2021

mandd Jul 21, 2021

wangcj05 Jul 26, 2021

mandd commented Jul 26, 2021

wangcj05 left a comment

		elif child.tag == 'variables':
		variables = [str(var.strip()) for var in child.text.split(",")]

Basic Event Scheduler Model #37

Are you sure you want to change the base?

Basic Event Scheduler Model #37

Conversation

mandd commented Jul 15, 2021

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 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

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

mandd commented Jul 26, 2021

wangcj05 left a comment

Choose a reason for hiding this comment