Mandd/multipleknapsack models #17

mandd · 2021-02-08T20:16:20Z

Creation of MultipleKnapsack model
Issue #18

wangcj05

some comments

wangcj05 · 2021-02-08T21:56:32Z

doc/user_manual/include/Knapsack.tex

+                 element1Val   ,element2Val   ,element3Val   ,element4Val   ,element5Val,
+                 element1Cost  ,element2Cost  ,element3Cost  ,element4Cost,element5Cost,
+                 validity,totalValue,capacityID</variables>
+      <capacity>capacityID</capacity>


In this case, capacity is required input/variable from RAVEN perspective, and you do not allow user to directly provide this value. Right?

This is correct, the goal is to have knapsack parameters as raven generated variables

wangcj05 · 2021-02-08T21:56:56Z

src/knapsack/BaseKnapsackModel.py

@@ -108,16 +110,19 @@ def run(self, container, inputDict):
      @ In, inputDict, dict, dictionary of inputs from RAVEN
    """
    totalValue = 0.0
+    capacity = inputDict[self.capacity][0]
+    print(capacity)


remove print?

wangcj05 · 2021-02-08T21:57:10Z

src/knapsack/BaseKnapsackModel.py

            totalValue = totalValue + inputDict[container.mapping[key][0]]
          else:
-            self.capacity   = self.capacity   - inputDict[container.mapping[key][1]]
+            print('======')


remove print?

wangcj05 · 2021-02-08T21:58:53Z

src/knapsack/MultipleKnapsackModel.py

+#Internal Modules End-----------------------------------------------------------
+
+
+class MultipleKnapsackModel(ExternalModelPluginBase):


Can you inherit directly from BaseKnapsackModel?

There is a subset of elements that are in common between these two classes.
We could design a baseClass for all knapsack models.
I am open to suggestions.

wangcj05 · 2021-02-08T22:01:05Z

src/knapsack/MultipleKnapsackModel.py

+    mapping = InputData.parameterInputFactory('map', contentType=InputTypes.StringType)
+    mapping.addParam('value', param_type=InputTypes.StringType, required=True)
+    mapping.addParam('cost',  param_type=InputTypes.StringType, required=True)
+    inputSpecs.addSub(mapping)


If you inherit directly from BaseKnapsackModel, most of these lines can be removed

yes, having a base class would simplify the structure of getInputSpecs and _readMoreXML methods.

wangcj05 · 2021-02-08T22:02:04Z

src/knapsack/MultipleKnapsackModel.py

+    container.mapping = {}
+    self.knapsackSet  = {}
+
+    for child in xmlNode:


update this with

specs = self.getInputSpecs()() specs.parseNode(xmlNode)

Fixed the overall _readMoreXML method

wangcj05 · 2021-02-08T22:05:33Z

src/knapsack/MultipleKnapsackModel.py

+    knapsackSetValues={}
+
+    # knapsackSetValues is a dictionary in the form {knapsackID: knapsackValue}
+    for knapsack in self.knapsackSet.keys():


check if self.knapsackSet[knapsack] in inputDict first?

check has been added

wangcj05 · 2021-02-08T22:32:50Z

src/knapsack/MultipleKnapsackModel.py

+          knapsackChosen = str(int(inputDict[key][0]))
+          testValue = knapsackSetValues[knapsackChosen] - inputDict[container.mapping[key][1]]
+          if testValue >= 0:
+            knapsackSetValues[knapsackChosen] = knapsackSetValues[knapsackChosen] - inputDict[container.mapping[key][1]][0]


This line can be moved to the outside of this if condition.

Code has been updated, similar issue was located in the base knapsack model which has been edited as well

wangcj05 · 2021-02-08T22:33:58Z

src/knapsack/MultipleKnapsackModel.py

+    else:
+      container.__dict__[self.outcome] = 0.
+
+    print(knapsackSetValues)


remove print?

wangcj05 · 2021-02-08T22:38:51Z

src/knapsack/MultipleKnapsackModel.py

+    numberUnsatConstraints = 0.0
+    for knapsack in knapsackSetValues.keys():
+      if knapsackSetValues[knapsack] < 0:
+        numberUnsatConstraints = numberUnsatConstraints + 1.


This loop can be combined with previous for loop.

Good point: merged with previous loop

mandd

Comment to reviewer

mandd · 2021-02-08T23:53:50Z

doc/user_manual/include/Knapsack.tex

+                 element1Val   ,element2Val   ,element3Val   ,element4Val   ,element5Val,
+                 element1Cost  ,element2Cost  ,element3Cost  ,element4Cost,element5Cost,
+                 validity,totalValue,capacityID</variables>
+      <capacity>capacityID</capacity>


This is correct, the goal is to have knapsack parameters as raven generated variables

mandd · 2021-02-08T23:54:15Z

src/knapsack/BaseKnapsackModel.py

@@ -108,16 +110,19 @@ def run(self, container, inputDict):
      @ In, inputDict, dict, dictionary of inputs from RAVEN
    """
    totalValue = 0.0
+    capacity = inputDict[self.capacity][0]
+    print(capacity)


mandd · 2021-02-08T23:54:21Z

src/knapsack/BaseKnapsackModel.py

            totalValue = totalValue + inputDict[container.mapping[key][0]]
          else:
-            self.capacity   = self.capacity   - inputDict[container.mapping[key][1]]
+            print('======')


mandd · 2021-02-08T23:59:12Z

src/knapsack/MultipleKnapsackModel.py

+#Internal Modules End-----------------------------------------------------------
+
+
+class MultipleKnapsackModel(ExternalModelPluginBase):


There is a subset of elements that are in common between these two classes.
We could design a baseClass for all knapsack models.
I am open to suggestions.

mandd · 2021-02-08T23:59:58Z

src/knapsack/MultipleKnapsackModel.py

+    mapping = InputData.parameterInputFactory('map', contentType=InputTypes.StringType)
+    mapping.addParam('value', param_type=InputTypes.StringType, required=True)
+    mapping.addParam('cost',  param_type=InputTypes.StringType, required=True)
+    inputSpecs.addSub(mapping)


yes, having a base class would simplify the structure of getInputSpecs and _readMoreXML methods.

mandd · 2021-02-09T00:08:39Z

src/knapsack/MultipleKnapsackModel.py

+    container.mapping = {}
+    self.knapsackSet  = {}
+
+    for child in xmlNode:


Fixed the overall _readMoreXML method

mandd · 2021-02-09T00:13:13Z

src/knapsack/MultipleKnapsackModel.py

+    knapsackSetValues={}
+
+    # knapsackSetValues is a dictionary in the form {knapsackID: knapsackValue}
+    for knapsack in self.knapsackSet.keys():


check has been added

mandd · 2021-02-09T00:42:53Z

src/knapsack/MultipleKnapsackModel.py

+          knapsackChosen = str(int(inputDict[key][0]))
+          testValue = knapsackSetValues[knapsackChosen] - inputDict[container.mapping[key][1]]
+          if testValue >= 0:
+            knapsackSetValues[knapsackChosen] = knapsackSetValues[knapsackChosen] - inputDict[container.mapping[key][1]][0]


Code has been updated, similar issue was located in the base knapsack model which has been edited as well

mandd · 2021-02-09T01:05:58Z

src/knapsack/MultipleKnapsackModel.py

+    numberUnsatConstraints = 0.0
+    for knapsack in knapsackSetValues.keys():
+      if knapsackSetValues[knapsack] < 0:
+        numberUnsatConstraints = numberUnsatConstraints + 1.


Good point: merged with previous loop

mandd · 2021-02-09T01:06:08Z

src/knapsack/MultipleKnapsackModel.py

+    else:
+      container.__dict__[self.outcome] = 0.
+
+    print(knapsackSetValues)


moosebuild · 2021-02-09T23:57:03Z

Job Test linux pull on 465135e : invalidated by @joshua-cogliati-inl

Civet recipe has been updated.

moosebuild · 2021-02-11T03:15:59Z

Job Test linux pull on 465135e : invalidated by @wangcj05

wangcj05

some comments.

wangcj05 · 2021-02-16T01:43:18Z

src/knapsack/BaseKnapsackModel.py

-
-    alias = InputData.parameterInputFactory('alias', contentType=InputTypes.StringType)
-    alias.addParam('variable', param_type=InputTypes.StringType, required=True)
-    alias.addParam('type', param_type=InputTypes.StringType, required=True)


I suggest to keep alias, this can be useful. The alias is handled directly by Model.py inside RAVEN without using InputData. If you want to keep this capability, we need to keep these three lines to let knapsack external model aware of it.

I removed because I thought I merely did a copy and paste; restored

wangcj05 · 2021-02-16T01:45:44Z

src/knapsack/BaseKnapsackModel.py

@@ -112,27 +101,4 @@ def run(self, container, inputDict):
      @ In, container, object, self-like object where all the variables can be stored
      @ In, inputDict, dict, dictionary of inputs from RAVEN
    """


You can make the run class as abstractmethod

wangcj05 · 2021-02-16T01:46:37Z

src/knapsack/SimpleKnapsackModel.py

+#Internal Modules End-----------------------------------------------------------
+
+
+class SimpleKnapsackModel(ExternalModelPluginBase):


This class needs to be updated with the base class.

yes, I'll updated the two derived classes

wangcj05 · 2021-02-16T01:47:03Z

src/knapsack/SimpleKnapsackModel.py

+      else:
+        raise IOError("BaseKnapsackModel: xml node " + name + " is not allowed")
+
+  def initialize(self, container, runInfoDict, inputFiles):


This one will be removed if you do not need it.

wangcj05

I have two extra comments.

wangcj05 · 2021-02-16T20:10:48Z

__init__.py

+from LOGOS.src.knapsack import MultipleKnapsackModel
+from LOGOS.src.knapsack import BaseKnapsackModel


You do not need to import BaseKnapsackModel since this model can not be directly used by RAVEN.

good point fixed

wangcj05 · 2021-02-16T20:12:14Z

doc/user_manual/include/Knapsack.tex

@@ -9,12 +9,13 @@ \section{Knapsack Models}
 find the optimal solution.


-\subsection{BaseKnapsackModel}
+\subsection{BaseKnapsack Model}


BaseKnapsackModel --> SimpleKnapsackModel? If so, please update this section.

good catch, fixed all section

…holab/LOGOS into mandd/MultipleknapsackModels

wangcj05 · 2021-02-16T20:55:24Z

Changes are good, and tests are green. This PR can be merged.

mandd added 9 commits February 5, 2021 18:26

added src files and tests

ef636b9

added test files

251920c

fixed files

9356a64

init deit

1ae2668

added doc section

c37bbc6

merge with devel

d56539c

delete whitespaces

c65556e

fixed files

fe314ac

delete whitespaces

eee6250

mandd requested a review from wangcj05 February 8, 2021 20:52

mandd added enhancement New feature or request priority normal labels Feb 8, 2021

fixed error

72c5a32

wangcj05 requested changes Feb 8, 2021

View reviewed changes

addressing reviewer's comments

6141ab1

mandd commented Feb 9, 2021

View reviewed changes

mandd added 3 commits February 8, 2021 18:19

delete whitespaces

b4dd50f

fixed typos

cb3b3b3

delete whitespaces

465135e

added base class

639ba75

wangcj05 requested changes Feb 16, 2021

View reviewed changes

mandd added 4 commits February 16, 2021 09:23

edits from base class

5e01fe3

works on base class

b072c64

edit tests file

f6ab6a5

delete whitespaces

9710ed2

wangcj05 requested changes Feb 16, 2021

View reviewed changes

mandd added 2 commits February 16, 2021 13:30

addressing comments

d4b0b12

Merge branch 'mandd/MultipleknapsackModels' of https://github.com/ida…

e0c2572

…holab/LOGOS into mandd/MultipleknapsackModels

wangcj05 approved these changes Feb 16, 2021

View reviewed changes

wangcj05 merged commit 2cf41f9 into devel Feb 16, 2021

		#Internal Modules End-----------------------------------------------------------


		class MultipleKnapsackModel(ExternalModelPluginBase):

		#Internal Modules End-----------------------------------------------------------


		class SimpleKnapsackModel(ExternalModelPluginBase):

		from LOGOS.src.knapsack import MultipleKnapsackModel
		from LOGOS.src.knapsack import BaseKnapsackModel

Mandd/multipleknapsack models #17

Mandd/multipleknapsack models #17

Conversation

mandd commented Feb 8, 2021 • edited Loading

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

mandd 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

moosebuild commented Feb 9, 2021

moosebuild commented Feb 11, 2021

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

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

wangcj05 commented Feb 16, 2021

mandd commented Feb 8, 2021 •

edited

Loading