Fix for None in state_to_latex latex output
#8273
Conversation
1ucian0
requested review from
a team,
nonhermitian,
nkanazawa1989 and
ikkoham
as code owners
|
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 3113876507
💛 - Coveralls |
There was a problem hiding this comment.
Actually, looking again, the premise of this fix isn't quite right, but that's because the current implementation is convoluted. The gist is that nonzero_indices purports to be the source of which items should be printed, but it's not: that's decided by whether the entry in latex_terms is not None. nonzero_indices is built by comparing to exactly 0, whereas the LaTeX output bits use a fuzzier comparison. The errors arise because those two things disagree: there should only be one source of truth. I think the LaTeX output here needs a bit of refactoring, if we're touching it now: we're just adding more warts to something that already looks like it's been complicated by several "bugfixes" not addressing the root cause.
|
Ill take it. The root of the problem is that |
There was a problem hiding this comment.
This seems like a simpler solution to the problem, thanks. I still don't like our data flow in this whole process, but this change at least makes things consistent and fixes the error without introducing any more complexity. Two minor comments, but otherwise this looks fine.
…ate-to-latex-ket/alternative1
|
|
||
|
|
||
| def num_to_latex_ket(raw_value: complex, first_term: bool) -> Optional[str]: | ||
| def num_to_latex_ket(raw_value: complex, first_term: bool, decimals: int = 0) -> Optional[str]: |
There was a problem hiding this comment.
I think a high number (e.g. 10 or 15) would be a more appropriate default here. I think it's unlikely someone would want to round amplitudes to 0 decimal places.
| @@ -1293,20 +1287,22 @@ def num_to_latex_ket(raw_value: complex, first_term: bool) -> Optional[str]: | |||
| return None | |||
|
|
|||
|
|
|||
| def numbers_to_latex_terms(numbers: List[complex]) -> List[str]: | |||
| def numbers_to_latex_terms(numbers: List[complex], max_size: int) -> List[str]: | |||
There was a problem hiding this comment.
I think this needs changing to decimals too, probably with a default value as well.
There was a problem hiding this comment.
good catch. done in cef4fb6
…ate-to-latex-ket/alternative1
1ucian0
added
the
Changelog: Bugfix
| @@ -1291,20 +1285,21 @@ def num_to_latex_ket(raw_value: complex, first_term: bool) -> Optional[str]: | |||
| return None | |||
|
|
|||
|
|
|||
| def numbers_to_latex_terms(numbers: List[complex]) -> List[str]: | |||
| def numbers_to_latex_terms(numbers: List[complex], decimals: int = 10) -> List[str]: | |||
There was a problem hiding this comment.
I was on the fence about asking for a release note for this new kwarg. I decided that we didn't need it because we don't re-export this function from the root of visualization or document it in the visualization api. But if people feel strongly that we should document this new kwarg we can always add it as part of the release prep.
* Add __init__.py to run tests for terra.states * Update tests to follow Qiskit/qiskit#8273 * Install matplotlib for plot_state_qsphere according to CI log * Install seaborn for plot_state_qsphere according to CI log * Fix suppressions for Jupyter warnings Co-authored-by: Hiroshi Horii <hhorii@users.noreply.github.com>
This is an alternative to #8174, with the same test.
Fixes #8169
Fixes #8073
Fixes #7513