Make marginal_counts act on Result memory

[peendebak]

Summary

The marginal_counts method only acted on the ExperimentResult.data.counts, not the ExperimentResult.data.memory field. This PR updates the method so that the memory is also marginalized. Fixes #7448

Details and comments

[coveralls]

Pull Request Test Coverage Report for Build 2015216391

• 16 of 16 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.004%) to 83.519%

---

Totals
Change from base Build 2011537247:	0.004%
Covered Lines:	52537
Relevant Lines:	62904

---

💛 - Coveralls

[mtreinish]

Thanks for pushing this up and sorry for the delay in an initial review. I think this is a good start, but I have a few inline comments that I think we should address. The biggest things are:

1. While this works correctly for the use case you wrote a test for, calling marginal_counts(result) on a result with memory will fail with a TypeError.

2. I also think we should put the marginalization of memory behind a feature flag (or maybe a separate function) because it is a change in behavior on a potentially inplace modification on a results object which might be unexpected for users. So we should make this opt-in somehow to not surprise people

[peendebak]

Thanks for pushing this up and sorry for the delay in an initial review. I think this is a good start, but I have a few inline comments that I think we should address. The biggest things are:

1. While this works correctly for the use case you wrote a test for, calling marginal_counts(result) on a result with memory will fail with a TypeError.
2. I also think we should put the marginalization of memory behind a feature flag (or maybe a separate function) because it is a change in behavior on a potentially inplace modification on a results object which might be unexpected for users. So we should make this opt-in somehow to not surprise people

@mtreinish I will pick up the inline comments. Initially my idea was that non marginalizing the memory is a bug and fixing it does not require backwards compatibility. But for inplace operations I agree that we should take some care. My proposal: add a flag marginalize_memory with options:

• marginalize_memory=True: marginalize both inplace and normal
• marginalize_memory=False: do not marginalize, and remove the memory field from the result as it is invalid (e.g. inconsistent with the counts)
• marginalize_memory=None: the old behaviour: just leave the memory intact

I would prefer to make True the default, but we could also leave None as the default for backwards compatibility.

[mtreinish]

Thanks for the quick update, the code LGTM now. I think you've covered all the issues in the previous version and the behavior is much more stable and clearly defined now. The only thing missing now is an upgrade section release note documenting the change in behavior around the memory field in the result when calling marginal_counts(). We need to document that the default behavior of maginal_counts() is changing from previous releases, why that change was made, and how users can restore the previous behavior (just by setting marginalize_memory=None) if that's what they need. Once that's in the release note file I think this is good to go.

[mtreinish]

LGTM, thank you for the prompt updates.