Add labels keyword argument to CQM.iter_constraint_data() #1382
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1382 +/- ##
==========================================
- Coverage 94.83% 93.69% -1.14%
==========================================
Files 98 84 -14
Lines 10427 7359 -3068
==========================================
- Hits 9888 6895 -2993
+ Misses 539 464 -75 ☔ View full report in Codecov by Sentry. |
| def iter_constraint_data(self, | ||
| sample_like: SamplesLike, | ||
| *, | ||
| labels: typing.Optional[typing.Iterable[typing.Hashable]] = None, |
There was a problem hiding this comment.
The types seem to already be imported.
| labels: typing.Optional[typing.Iterable[typing.Hashable]] = None, | |
| labels: Optional[Iterable[Hashable]] = None, |
There was a problem hiding this comment.
I am deliberately using the fully qualified typing. At some point I'll go through this and remove all of those names imported into the namespace and replace them with typing. but for now I wanted to minimize changes. I suppose there is an argument for local consistency, but 🤷
There was a problem hiding this comment.
Fair enough. I guess both is fine, although I generally find using typing. everywhere makes the types less legible, although there are benefits to it as well. Python's typing system could definitely be better.
There was a problem hiding this comment.
I agree it's more cluttered visually. But I (personally) prefer cluttered signatures to a cluttered namespace. Determining what's a local symbol vs an imported one is much nicer with the qualified names. With typing most folks probably know which is which, but I prefer to be explicit when possible.
| @@ -1219,6 +1233,7 @@ def iter_constraint_data(self, sample_like: SamplesLike) -> Iterator[ConstraintD | |||
| def iter_violations(self, sample_like: SamplesLike, *, | |||
| skip_satisfied: bool = False, | |||
| clip: bool = False, | |||
| labels: typing.Optional[typing.Iterable[typing.Hashable]] = None, | |||
There was a problem hiding this comment.
| labels: typing.Optional[typing.Iterable[typing.Hashable]] = None, | |
| labels: Optional[Iterable[Hashable]] = None, |
| @@ -1153,6 +1157,7 @@ def iter_constraint_data(self, sample_like: SamplesLike) -> Iterator[ConstraintD | |||
| Args: | |||
| sample_like: A sample. `sample-like` is an extension of | |||
| NumPy's array_like structure. See :func:`.as_samples`. | |||
| labels: A subset of the constraint labels over which to iterate. | |||
There was a problem hiding this comment.
Might be worth mentioning that the constraints will be yielded in the same ordering as the labels passed, instead of in order of addition which I presume is the case without a labels subset request.
There was a problem hiding this comment.
I am being deliberately ambiguous in case we want to change the behavior later.
See #1381. This addresses one of the options laid out.