-
Notifications
You must be signed in to change notification settings - Fork 65
Add problem info to the NL sampler result #565
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
Add problem info to the NL sampler result #565
Conversation
`info` field is a dict that includes timing info, and warnings pulled out of the timing.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #565 +/- ##
==========================================
- Coverage 88.88% 86.08% -2.81%
==========================================
Files 21 21
Lines 1809 1818 +9
==========================================
- Hits 1608 1565 -43
- Misses 201 253 +52 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
would be nice if this repo had release notes for such updates
Co-authored-by: Joel Pasvolsky <[email protected]>
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
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!
@@ -918,7 +918,7 @@ def parameters(self) -> Dict[str, List[str]]: | |||
|
|||
class SampleResult(NamedTuple): | |||
model: dwave.optimization.Model | |||
timing: dict | |||
info: dict |
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 suppose we could say
info: dict[str, typing.Any]
might be overkill but 🤷
Even more overkill would be to use typing.Literal
.
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.
And how about this? 😃
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 annotated!
Close #528.
Opted for the second option from #528, renaming
timing
toinfo
and adding general problem info there, together with timing info and warnings.The
info
field returned inLeapHybridNLSampler.SampleResult
named tuple is adict
, now consistent withSampleSet.info
, e.g.:This change is technically breaking backwards-compatibility, but given that:
LeapHybridNLSampler.sample()
hasn't been documented in full (name of thetiming
field is not specified),I think we should be OK making this change.