Fix flaky REM test / refactor #2464
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2464 +/- ##
=======================================
Coverage 98.70% 98.70%
=======================================
Files 88 88
Lines 4083 4091 +8
=======================================
+ Hits 4030 4038 +8
Misses 53 53 ☔ View full report in Codecov by Sentry. |
| abs_tol=0.1, | ||
| ) | ||
| @pytest.mark.parametrize("repeats", range(10)) | ||
| def test_probability_vector_roundtrip(repeats): |
There was a problem hiding this comment.
Nit: To make it clear that the actual value of repeats is not actually important in the test body, you can instead use _ as argument.
| probability_vector: npt.NDArray[np.float64], samples: int | ||
| ) -> List[Bitstring]: | ||
| probability_vector: Sequence[float], samples: int | ||
| ) -> list[str]: | ||
| """Generate a number of samples from a probability distribution as | ||
| bitstrings. |
There was a problem hiding this comment.
It could be useful to add an example of input --> output for our future selves.
There was a problem hiding this comment.
Considering this left us a bit confused when first encountering it, I think that's a great idea. Done in f8725eb.
| probability_vector: npt.NDArray[np.float64], samples: int | ||
| ) -> List[Bitstring]: | ||
| probability_vector: Sequence[float], samples: int | ||
| ) -> list[str]: |
There was a problem hiding this comment.
I am curious as to what's usually preferred when we specify the return type?
I see you have used list[str] here but you have also used from typing import List before specifying the return type as List[str] on another line.
Searching around for this shows that we do not need to use the typing module for >= Python 3.9. Do we need to add this as a to-do for other modules in mitiq?
There was a problem hiding this comment.
Didn't we have this exact same discussion with @vprusso in another PR? iirc there we learned that list[str] is the way to go.
There was a problem hiding this comment.
Yes, we did. Which is why I was asking if we need to add this to our todos.
We still use things like from typing import List elsewhere.
There was a problem hiding this comment.
We don't need to be so proactive. The aliases are deprecated, but they are unlikely to be removed in the foreseeable future.
There was a problem hiding this comment.
Agreed, we should not take action until we need to on this. In the meantime lets try to use the builtins instead of typing.List, but it's not a big concern since they both achieve the same goal.
If we do migrate eventually, we will have to import a large majority of the types from collections.abc since many of the types we rely on (e.g. typing.Sequence) are deprecated in favor of coming from collections.abc.
Description
fixes #2431 by setting a random seed to ensure deterministic test.
I also took the liberty to refactor the function at hand which looked to be more complicated than needed.
cc @FarLab @jordandsullivan @bdg221 since we got this started in the mitiq coding call together.