-
Notifications
You must be signed in to change notification settings - Fork 26
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
Reimplement demo #25
Reimplement demo #25
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good so far.
As you said, we should remove "interfaces", jsonschema, maybe unify drawing (with saving to file)..
I like the new CLI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
||
if region == 'global' and sampler_type == 'qpu': | ||
print("Given region is too large for the QPU, please choose another " | ||
"region or use hybrid.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So when running python structural_imbalance.py --qpu
which is a likely choice for new users wanting to use the quantum computer, the demo will print "please choose another region or use hybrid" but keep running?
a. Should it not stop so the user can follow this message?
b. The user used the default region, did not chose a region, so maybe something like, "please chose a smaller region..."
Why not default to QPU and Syria?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, this is the only sentence that has a period at the end. Please be consistent in following or violating the rules of grammar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The future is hybrid! I think the reason that this demo is interesting relative to the illustrated demo is because it lets users run on a larger data set. That's why I default it to global/hybrid.
sampler = EmbeddingComposite(DWaveSampler()) | ||
|
||
else: | ||
raise RuntimeError("unknown solver type") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative to consider is to make this like the region with given choices for sampler. That would make it clear to new users that they are selecting a sampler as they need to do in their code:
Options:
-- sampler [qpu|cpu|hybrid]
--region [global|iraq|syria]
The README would explain the samplers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to be consistent with the existing interface. But this would work as well.
help="Use Leap's hybrid sampler") | ||
@click.option('--region', default='global', | ||
type=click.Choice(['global', 'iraq', 'syria'], | ||
case_sensitive=False)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
['global', 'iraq', 'syria'] --> ['global', 'Iraq', 'Syria']
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
case_sensitive=False
😄
# use the chosen sampler (passing in the parameters) | ||
edges, colors = dnx.structural_imbalance(G, sampler, **params) | ||
|
||
print("Found", len(edges), 'violations out of', len(G.edges), 'edges') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This printout will be mistaken for an error message.
Maybe something like "Found a best partition of ", len(G.edges), 'edges with an imbalance of', len(edges), 'edges'
Simplify and demo-fy.
So far I have barely touched the current demo but would remove the rest of it as part of this PR. Wanted to get review on the draft before proceeding though.
Obviously needs a bit more documentation, but should be clear enough.
Closes #23