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

penaltymodel-maxgap energy levels #76

Merged
merged 6 commits into from
Jan 17, 2019

Conversation

arcondello
Copy link
Member

Closes #73

Copy link
Contributor

@m3ller m3ller left a comment

Choose a reason for hiding this comment

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

It looks good overall. I just had some small suggestions.

@@ -57,6 +70,9 @@ def generate_ising(graph, feasible_configurations, decision_variables,
to a non-zero infeasible gap.

"""
if len(graph) == 0:
return dimod.BinaryQuadraticModel.empty(dimod.SPIN), float('inf')
Copy link
Contributor

Choose a reason for hiding this comment

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

When MIP finds that it is given a table with no feasible configurations, it returns a zero-ed out BQM and a gap of zero. Here, MaxGap is returning an empty model and a gap of infinity. Perhaps we should have a consistent default gap when bad argument is given?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea. What do you think makes more sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

I like a gap of infinity because I think it's unlikely that a user would choose that as a gap. Hence, when you see it's an infinity you know it's an error or a computer generated default.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is out-of-scope for this PR then. See #79

penaltymodel_maxgap/penaltymodel/maxgap/theta.py Outdated Show resolved Hide resolved
penaltymodel_maxgap/tests/test_generation.py Show resolved Hide resolved
* remove six dependency
* refactor Theta class to use dimod's BQM object for logic
* change generate_ising to generate which now returns a BQM
* refactor unittests
* fix offset bug
BQMs in which every configuration have a specified energy should
be treated as having infinite gap.

Reasoning:
* It tests against gap>0 which is a good shorthand for a valid
penalty model
* The intuition is that if one increased energy from ground and
stops when the first infeasible state is reached is maintained. In
that case the energy would increase -> inf
@codecov-io
Copy link

codecov-io commented Jan 17, 2019

Codecov Report

Merging #76 into master will decrease coverage by 0.09%.
The diff coverage is 97.6%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #76     +/-   ##
=========================================
- Coverage   96.04%   95.94%   -0.1%     
=========================================
  Files          26       28      +2     
  Lines        1389     1358     -31     
=========================================
- Hits         1334     1303     -31     
  Misses         55       55
Impacted Files Coverage Δ
...tymodel_maxgap/penaltymodel/maxgap/package_info.py 100% <ø> (ø) ⬆️
penaltymodel_maxgap/tests/test_smt.py 100% <ø> (ø) ⬆️
...altymodel_maxgap/penaltymodel/maxgap/generation.py 100% <100%> (ø) ⬆️
penaltymodel_maxgap/tests/test_theta.py 100% <100%> (ø)
penaltymodel_maxgap/penaltymodel/maxgap/smt.py 98.4% <100%> (+1.14%) ⬆️
...naltymodel_maxgap/penaltymodel/maxgap/interface.py 87.5% <75%> (-12.5%) ⬇️
penaltymodel_maxgap/penaltymodel/maxgap/theta.py 97.56% <97.56%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b34b781...661cefe. Read the comment docs.

Copy link
Contributor

@m3ller m3ller left a comment

Choose a reason for hiding this comment

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

Looks good. Just noticed a small typo.

Provides the structure for Theta.

linear_energy_ranges (dict):
A dict of the form {v: (min, max, ...} where min and max are the
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing closing parethesis. v: (min, max)

@m3ller m3ller merged commit b7b8cfe into dwavesystems:master Jan 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants