Add more conditions for checking `is_separable`

Yash-10 commented 1 month ago

Description

This is for issue #518

Changes

Notable changes that this PR has either accomplished or will accomplish. Feel free to add more lines to the itemized list below.

[X] Added several conditions porting from the MATLAB implementation. I believe only 1-2 relative checks (found to be slower, as written in the MATLAB code comments) are omitted.
[X] Added print statements to discern the reason for a specific output (True/False) for transparency.
[X] Initiated tests for the conditions added here. As discussed in #518, designing the test cases is not trivial, but we will try to work on it based on our potential discussion here.
[X] I think there was a bug in the original source code in this line: if (lam[0] - lam[2 * max_dim - 1]) ** 2 <= 4 * lam[2 * max_dim - 2] * lam[2 * max_dim] + tol**2: since Python's indexing is different than MATLAB. That has been changed here.

Checklist

Before marking your PR ready for review, make sure you checked the following locally. If this is your first PR, you might be notified of some workflow failures after a maintainer has approved the workflow jobs to be run on your PR.

Additional information is available in the documentation.

[X] Use ruff for errors related to code style and formatting.
[X] Verify all previous and newly added unit tests pass in pytest.
[x] Check the documentation build does not lead to any failures. Sphinx build can be checked locally for any failures related to your PR
[x] Use linkcheck to check for broken links in the documentation
[ ] Use doctest to verify the examples in the function docstrings work as expected.

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 47.36842% with 30 lines in your changes missing coverage. Please review.

Project coverage is 97.6%. Comparing base (c493fd4) to head (8e6d3c4).

Files	Patch %	Lines
toqito/state_props/is_separable.py	47.3%	25 Missing and 5 partials :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #612 +/- ## ======================================== - Coverage 98.1% 97.6% -0.5% ======================================== Files 162 162 Lines 3166 3215 +49 Branches 771 787 +16 ======================================== + Hits 3108 3141 +33 - Misses 37 48 +11 - Partials 21 26 +5 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Yash-10 commented 1 month ago

Errors related to code format have been (hopefully) resolved now. I've been able to add two working tests: test_separable_schmidt_rank and test_separable_based_on_eigenvalues. I've cross-checked both with QETLAB's output (both the True/False output and the reason).

However, as we have been discussing before, adding tests for the other cases (especially since most of the conditions added in this PR come after the already-existing checks above in the is_separable function) seems challenging but also an interesting task!

@purva-thakre suggested this idea in #612:

My plan for finding additional examples (test cases) was to go through the references listed in the docstring of the QETLAB function.

I am trying to mine through these references. So far, some examples I found return an output from a condition above the one I wanted to check. If there are any suggestions regarding designing these tests, I will be very happy to know about them!

One idea: Should we modify is_separable to optionally allow returning the reason (a condensed string representation)? Something like return_reason=True. An advantage is that the tests can use these to ascertain the intended condition has been checked and returned. But perhaps this can be tackled at a later point.

purva-thakre commented 1 month ago

One idea: Should we modify is_separable to optionally allow returning the reason (a condensed string representation)? Something like return_reason=True. An advantage is that the tests can use these to ascertain the intended condition has been checked and returned. But perhaps this can be tackled at a later point.

@Yash-10 I think we discussed this in the corresponding issue. Temporarily add print comments to check which parts of the code are being used for which unit test. The in-line comments are more than enough.

If it's too daunting of a task to search for the examples, we could review your code, merge this PR for your bounty, and I can worry about adding the unit tests later. @vprusso WDYT?

Yash-10 commented 1 month ago

If it's too daunting of a task to search for the examples, we could review your code, merge this PR for your bounty, and I can worry about adding the unit tests later. @vprusso WDYT?

Thanks. I was thinking of searching a bit more and seeing if I can find examples within the coming day or two. If not, we can decide what can be done about this PR. For example, we can merge this as you said, and then keep adding these tests as a TODO (in a separate issue). If that's the case, a temporary option I had in mind was only to use the code I added and compare it with QETLAB rather than comparing it with the output of is_separable. This might require copying and pasting the code also in the test scripts. This can be removed once the proper tests are added. Please let me know if this seems plausible, we can do that, but maybe let me explore a bit, and I'll comment tomorrow about my updates.

If you'd like to review the PR in the meanwhile, please feel free to do so.

vprusso commented 1 month ago

If it's too daunting of a task to search for the examples, we could review your code, merge this PR for your bounty, and I can worry about adding the unit tests later. @vprusso WDYT?

Thanks. I was thinking of searching a bit more and seeing if I can find examples within the coming day or two. If not, we can decide what can be done about this PR. For example, we can merge this as you said, and then keep adding these tests as a TODO (in a separate issue). If that's the case, a temporary option I had in mind was only to use the code I added and compare it with QETLAB rather than comparing it with the output of is_separable. This might require copying and pasting the code also in the test scripts. This can be removed once the proper tests are added. Please let me know if this seems plausible, we can do that, but maybe let me explore a bit, and I'll comment tomorrow about my updates.

If you'd like to review the PR in the meanwhile, please feel free to do so.

Hi @Yash-10 ,

Thank you again for your interest and current efforts on this issue!

Regarding your points, I think that makes sense--that is, to compare the outputs of your code with QETLAB to ensure that the logic is hitting the same points. Copying/pasting test scripts on a branch is all good as we don't need to necessarily commit that, and this is a process I've used in the past to check my functions against some of the functions from QETLAB.

For now, as your branch is in flux, I'll hold off on putting in any review as I know there are quite a few TODO stubs. Of course, let me know if that changes or if you'd prefer I review it anyway.

Don't hesitate to reach out again with any comments/questions!

Yash-10 commented 1 month ago

Hello @vprusso, thank you for your comment! I've been able to add a few more tests since yesterday. I've removed the TODOs. It would be great if you could review it since it might also pinpoint where this PR is with the initial aim of #518 and what is needed more to accomplish those goals. Thanks!

Update: I've added two comments, which are changes in the code from the already-existing code in the repo.

purva-thakre commented 1 month ago

@Yash-10 Could you please remove the print statements? This temporary suggestion was for you to understand where the function was stopping before returning True or False.

I can take a stab at reviewing this PR after that.

Yash-10 commented 1 month ago

I still do think that optionally allowing returning the reason (a small short description) from is_separable might assist those wanting more transparency, but since @purva-thakre did not like the idea, I've removed the print statements. You can review it now.

purva-thakre commented 1 month ago

I still do think that optionally allowing returning the reason (a small short description) from is_separable might assist those wanting more transparency

All suggestions are welcome! If we make this change, we will have to make similar changes to other functions for the sake of consistency. It will be a major refactor along with the added responsibility of tracking down the references for each conditional loop of a function.

We have quite a few is_some_property functions in the following modules:

toqito/channel_props
toqito/matrix_props
toqito/state_props
toqito/measurement_props

Yash-10 commented 1 month ago

If we make this change, we will have to make similar changes to other functions for the sake of consistency. It will be a major refactor along with the added responsibility of tracking down the references for each conditional loop of a function. We have quite a few is_some_property functions in the following modules:

toqito/channel_props

toqito/matrix_props

toqito/state_props

toqito/measurement_props

I see. It might indeed be a big refactor. If required, I can raise a separate issue for this.

Yash-10 commented 1 month ago

Is any additional change required to realize the completion of this PR @purva-thakre? If so, I can use the time to make any amendments. Thanks!

vprusso commented 1 month ago

Okay, modulo any remaining comments from @purva-thakre and the doctest test, I believe things look good on my end. @Yash-10 let me know if I'm missing any question that slipped under my radar!

Yash-10 commented 1 month ago

Thank you, @vprusso, for your detailed comments and suggestions! I've (hopefully) replied to all of them whenever necessary. To summarize, several conditions have been added but quite a few lines are still uncovered. I'll create an issue to summarize this soon.

Also, just so we are aware, at least two tests return the expected value, but not from the condition the test was designed for, according to me:

test_entangled_cross_norm_realignment_criterion
test_separable_based_on_eigenvalues (this was added by me in this PR)

About your point:

And also, this check that does not appear to be doing anything?

  if (
      state_rank + np.linalg.matrix_rank(pt_state_alice)
      <= 2 * state.shape[0] * state.shape[1] - state.shape[0] - state.shape[1] + 2
      or state_rank + np.linalg.matrix_rank(pt_state_bob)
      <= 2 * state.shape[0] * state.shape[1] - state.shape[0] - state.shape[1] + 2
  ):
      # Determined to be separable via operational criterion of the PPT criterion for low-rank operators.
      # P. Horodecki, M. Lewenstein, G. Vidal, and I. Cirac.
      # Operational criterion and constructive checks for the separability of low-rank density matrices.
      # Phys. Rev. A, 62:032310, 2000.
      # TODO
      pass

The implementation here differs from QETLAB. In essence, the cited paper suggests a separability criterion if the condition in the above code block holds true. Although implementing it will take some time.

purva-thakre commented 1 month ago

Congrats on the bounty @Yash-10 !

vprusso commented 1 month ago

Yes, congrats on the bounty, @Yash-10 and thank you for the additional reviews and checks, @purva-thakre !

vprusso / toqito