Skip to content
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

Fix result print for SamplingVQE & QAOA - fixes #58 #65

Merged
merged 4 commits into from
Aug 23, 2023
Merged

Fix result print for SamplingVQE & QAOA - fixes #58 #65

merged 4 commits into from
Aug 23, 2023

Conversation

woodsp-ibm
Copy link
Member

@woodsp-ibm woodsp-ibm commented Aug 22, 2023

Summary

Fixes #58.

Simply removes the overriden __str__ method in the sampling result class so conversion to string is done by the parent AlgorithmResult (as it done for all other results so it makes things consistent too).

Longer terms the change proposed in #53 will change things up by using dataclass so the result string format is wanted to be improved, maybe include a capability for just including the common fields or whatever things can be revisited at that point.

Details and comments

Was

>>> res = SamplingVQEResult()
>>> print(res)
SamplingMinimumEigensolverResult:
        Eigenvalue: None
        Best measurement
: None

Now

>>> res = SamplingVQEResult()
>>> print(res)
{   'aux_operators_evaluated': None,
    'best_measurement': None,
    'cost_function_evals': None,
    'eigenstate': None,
    'eigenvalue': None,
    'optimal_circuit': None,
    'optimal_parameters': None,
    'optimal_point': None,
    'optimal_value': None,
    'optimizer_evals': None,
    'optimizer_result': None,
    'optimizer_time': None}

@woodsp-ibm woodsp-ibm added this to the 0.2.0 milestone Aug 22, 2023
@coveralls
Copy link

coveralls commented Aug 22, 2023

Pull Request Test Coverage Report for Build 5953737313

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.05%) to 90.038%

Totals Coverage Status
Change from base Build 5953372270: 0.05%
Covered Lines: 6444
Relevant Lines: 7157

💛 - Coveralls

Copy link
Collaborator

@ElePT ElePT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that @Cryoris's original motivation to override the __str__ method was not to overload the users with non-relevant outputs that "blow-up" exponentially with the number of qubits. I see his point there. At the same time, I think that it would make sense to expose outputs such as the optimal point, and that it is not ideal to have such a specific override of an else-general format among algorithms. Given that this very specific override might be confusing to some users, and that exceptions are harder to keep up with/ maintain, I believe the best way to go here is to merge this PR, but I am open to future refactorings of AlgorithmResult to improve readability of the converted string.

@mergify mergify bot merged commit f433cf0 into qiskit-community:main Aug 23, 2023
15 checks passed
@woodsp-ibm woodsp-ibm deleted the fixes_58 branch August 23, 2023 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[from qiskit] SamplingMinimumEigensolverResult and subclasses thereof when printing
3 participants