Skip to content
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

Mandd/knapsack models #8

Merged
merged 8 commits into from
Feb 4, 2021
Merged

Mandd/knapsack models #8

merged 8 commits into from
Feb 4, 2021

Conversation

mandd
Copy link
Collaborator

@mandd mandd commented Feb 3, 2021

It is needed to develop Knapsack models that can be used coupled with RAVEN when
the desired optimization problem requires the use of specific models to generate
knapsack required parameters.
Ref. Issue #7

@mandd mandd requested a review from wangcj05 February 3, 2021 23:45
@mandd mandd added enhancement New feature or request priority normal labels Feb 3, 2021
Copy link
Collaborator

@wangcj05 wangcj05 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some comments.

LOGOS contains a set of Knapsack models that can be used coupled with RAVEN when
the desired optimization problem requires the use of specific models to generate
knapsack required parameters.
More specifically, all these models would be contained in a RAVEN ensembleModel
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ensembleModel --> EnsembleModel

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

# 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.
"""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update the copyright statement, this one is not consistent with the version that we are using.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

pass


def run(self, container, Inputs):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change "Inputs" to "inputs" or "inputDict"

if key in Inputs.keys() and Inputs[key] in [0.0,1.0]:
if Inputs[key] == 1.0:
testValue = self.capacity - Inputs[container.mapping[key][1]]
if testValue > 0:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

testValue > 0 --> testValue >= 0?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ouch, good catch!

@@ -0,0 +1,110 @@
<Simulation verbosity="debug">
<TestInfo>
<name>[]</name>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update name info

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

elif child.tag == 'map':
container.mapping[child.text.strip()] = [child.get('value'),child.get('cost')]
elif child.tag == 'variables':
variables = [str(var.strip()) for var in child.text.split(",")]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the External Model for RAVEN, I think we'd better to use InputData and InputTypes to process the xml input.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I may need some help here

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have checked other External Models inside LOGOS, they are not using the InputData yet. If you want to use InputData for LOGOS, I suggest we open an issue, and adapt the InputData in another PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue #9 has been opened.
We should have a chat on how to fix it.

Copy link
Collaborator Author

@mandd mandd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressing reviewer comments

LOGOS contains a set of Knapsack models that can be used coupled with RAVEN when
the desired optimization problem requires the use of specific models to generate
knapsack required parameters.
More specifically, all these models would be contained in a RAVEN ensembleModel
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

# 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.
"""
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

elif child.tag == 'map':
container.mapping[child.text.strip()] = [child.get('value'),child.get('cost')]
elif child.tag == 'variables':
variables = [str(var.strip()) for var in child.text.split(",")]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I may need some help here

if key in Inputs.keys() and Inputs[key] in [0.0,1.0]:
if Inputs[key] == 1.0:
testValue = self.capacity - Inputs[container.mapping[key][1]]
if testValue > 0:
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ouch, good catch!

@@ -0,0 +1,110 @@
<Simulation verbosity="debug">
<TestInfo>
<name>[]</name>
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

@mandd
Copy link
Collaborator Author

mandd commented Feb 4, 2021

Tests are green on my local machine:
Screen Shot 2021-02-04 at 4 01 50 PM

Documentation compiles as well without errors:
Screen Shot 2021-02-04 at 4 04 45 PM

@wangcj05
Copy link
Collaborator

wangcj05 commented Feb 4, 2021

This PR is good to be merged.

@wangcj05 wangcj05 merged commit cc134f3 into master Feb 4, 2021
@wangcj05 wangcj05 mentioned this pull request Feb 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority normal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants