Add BaseResult functionality to primitives module

[pedrorrivero]

Summary

Adds a primitive result base class to provide common functionality to all inheriting result dataclasses.

Closes #8089
Changelog: New Feature

Details and comments

• Adds result.num_experiments property.
• Adds result.experiments property.
• Checks for consistency in the number of experiments across data fields after instantiation (i.e. on __post_init__).

To do

• Translate tests from pytest to unittest
• Add release note
• Complete docstring (e.g. autosummary)

[qiskit-bot]

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:

• @Qiskit/terra-core
• @ajavadia
• @ikkoham
• @levbishop
• @t-imamichi

[coveralls]

Pull Request Test Coverage Report for Build 3051563525

• 40 of 40 (100.0%) changed or added relevant lines in 4 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.01%) to 84.347%

---

Totals
Change from base Build 3050113514:	0.01%
Covered Lines:	57998
Relevant Lines:	68761

---

💛 - Coveralls

[ikkoham]

All of the features are useful and they are great from a code standpoint. I leave some minor comments. But, from an interface point of view, it would be good to have @ajavadia and @levbishop review.

[ikkoham]

Thanks. LGTM. But I would like to wait more comments.

[t-imamichi]

Looks good to me too.

[blakejohnson]

In general, I think the name experiments is a poor choice. In the process of developing OpenQASM3, we proposed a generalized "circuit" concept that supplants what has previously been called an "experiment" in terra. This word is also overloaded by the "experiment" concept in qiskit-experiments.

I realize that there are many residual uses of "experiments" in terra, but would it make sense to already start the move away by settling on "circuits" here? i.e. num_circuits and circuits as the property names?

[pedrorrivero]

I don't think that would work @blakejohnson . Some of the primitives (if not all) currently hold a list of circuits that they can use to run "experiments". That means that the user can request n different experiments on anyone of theses circuits (e.g. for different observables in Estimator), and at that point num_circuits will become misleading.

I am very curious to learn how this distinction is being handled in OpenQASM3.

I have no personal attachment to the word "experiment" though, and I'll be happy to change it if we come up with a better alternative. What I like about it though, is that it conveys the fact that we are referring to something atomic; as opposed to "job", for example, which can encapsulate several of these atomic elements.

[ikkoham]

LGTM (need to fix lint and tests). Thank you! (I was about to point out that base_result does not use __future__.annotations, but you already fixed it.)

[ikkoham]

We cannot see the docs of BasePrimitivesResult because it's not indexed. so you need to add BasePrimitivesResult to the module docstrings in primitives/__init__.py.

[ikkoham]

LTGM, Thanks!

[ikkoham]

I want to wait @t-imamichi's approval.

[t-imamichi]

I'm wondering whether the check isinstance(value, str) is enough or not. What if bytes object is given?

[t-imamichi]

Should we check each entry of Sequence perhaps? Do you have any suggestion, @ikkoham?

[pedrorrivero]

The problem with checking each sequence entry is that the types will differ for each data attribute (e.g. values and metadata), and between primitives (e.g. sampler and estimator). So those checks should probably live under the particular primitives' __post_init__ methods (e.g. Estimator.__post_init__), instead of inside BasePrimitiveResult.

Is this what you are referring to?

[t-imamichi]

I see. I just wondered whether there is any simpler way of this check.

[t-imamichi]

This PR looks good overall. I have a minor comment. I'm wondering about the validation check of the data. #8091 (comment)

[pedrorrivero]

I have already excluded bytes from the possible types (@t-imamichi was correct, str was not enough).

I don't think that this type check is comprehensive though (surely better than no check at all), but the best approach to solve it should be by enforcing an specific type (e.g. tuple) as proposed in #8105

[t-imamichi]

Thank you, @pedrorrivero. This PR looks good now.
I also have the same impression that the type check might not be comprehensive. But I don't have a good idea. We can leave it as future work.

[ikkoham]

Ok, let's merge this PR!