Fix Variables._relabel() with supersets#1409
Conversation
| """ | ||
| # first if the mapping is a superset of existing, we can go ahead and drop | ||
| # the stuff we don't care about | ||
| mapping = {old: new for old, new in mapping.items() if old in existing} |
There was a problem hiding this comment.
This is exactly the workaround I implemented in dwavesystems/dwave-system#582. I was hoping it's feasible to fix it on a "lower level" (but I haven't checked where the actual bug is).
There was a problem hiding this comment.
The bug is here:
Line 241 in 7c34d7a
We pop 1, which results in a miss but then 1 gets assigned to idx. The first fix that I implemented was to ignore it here, but IMO the upstream fix that I went with ends up being better. At the cost of a copy (which as I said, is one of many).
There was a problem hiding this comment.
The fix I went with also makes iter_safe_relabels() (a public utility function) more correct than it was before.
|
The doctest failure is unrelated. I will also fix that in a followup PR. |
2 participants
Looking through the logic, I think it would be possible to avoid the copy with a more significant rework. But we already are doing many copies and traversals in that family of functions so IMO the simpler approach is best.
Closes #1408