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

Implement and test process fidelity #1712

Merged

Conversation

fhopfmueller
Copy link
Contributor

Description
This PR implements and tests a new version of process_fidelity.
The new version follows the definition in Gilchrist et al., Physical Review A 71, 062310 (2005).

Related issues or PRs
Discussion at #1703.
I previously submitted a similar PR at #1708, targeting master.
This PR targets dev.major rather than master because existing functionality is changed, and contains improvements following @Ericgig 's suggestions. I have also adapted the tests to match the style of the tests in dev.major.

Addresses part 2 of #1703.

Changelog
Implement and test a new version of process_fidelity (#1703)

@coveralls
Copy link

coveralls commented Nov 15, 2021

Coverage Status

Coverage increased (+0.07%) to 65.504% when pulling cd831bc on fhopfmueller:dev.major-fix-process-fidelity into 8ac4518 on qutip:dev.major.

Copy link
Member

@Ericgig Ericgig left a comment

Choose a reason for hiding this comment

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

Looks good.
It's great to have a proper documentation, tests, and a properly defined function.
_process_fidelity_to_id makes it easier to read.

Since the definition of the function changed, could you add a warning. We need to be clear that when going from v4 to v5, the result will change and we can't expect everybody will read the documentation.

qutip/core/metrics.py Outdated Show resolved Hide resolved
qutip/tests/core/test_metrics.py Outdated Show resolved Hide resolved
- Refactor tests of process_fidelity using pytest.mark.parametrize
- In one branch of process_fidelity, divide fidelity by dimensional factor instead of dividing density matrices
Copy link
Member

@Ericgig Ericgig left a comment

Choose a reason for hiding this comment

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

Seems good to me.
Thank you fhopfmueller.

Copy link
Contributor

@hodgestar hodgestar left a comment

Choose a reason for hiding this comment

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

This looks great. Thank you!

qutip/core/metrics.py Show resolved Hide resolved
qutip/core/metrics.py Show resolved Hide resolved
qutip/core/metrics.py Show resolved Hide resolved
qutip/core/metrics.py Show resolved Hide resolved
qutip/core/metrics.py Outdated Show resolved Hide resolved
fhopfmueller and others added 3 commits November 27, 2021 18:46
Co-authored-by: Simon Cross <hodgestar+github@gmail.com>
Co-authored-by: Simon Cross <hodgestar+github@gmail.com>
Co-authored-by: Simon Cross <hodgestar+github@gmail.com>
@hodgestar hodgestar added this to the QuTiP 5.0 milestone Dec 1, 2021
@hodgestar
Copy link
Contributor

@fhopfmueller Thank you for making this PR happen! Merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants