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

Fixes and tests for apply! #144

Merged
merged 2 commits into from
Sep 18, 2023
Merged

Fixes and tests for apply! #144

merged 2 commits into from
Sep 18, 2023

Conversation

Krastanov
Copy link
Collaborator

A previous pull request fixed a few issues with apply!, but there were still a number of edge cases that were not covered. This addresses such cases, simplifies the checks for whether an embedding is necessary, and properly detects permutations of the subsystem indices.

@Krastanov
Copy link
Collaborator Author

@Abhishek-1Bhatt , could you review this, as it addresses code you worked on?

I also modified some of your tests to use @test_throws instead of a try-catch. The try-catch is not reliable here because one day the methods could have just stopped throwing and the tests would not have caught that.

It does not use the expensive permutesystems anymore, but more importantly, it fixes the following regression from the previous pull request: apply!(state_with_3_subsystems, [1,2], operator_with_2_subsystems) was broken because it was calling the meaningless permutesystems(operator_with_3_subsystems, [1,2]).

@codecov
Copy link

codecov bot commented Sep 18, 2023

Codecov Report

Merging #144 (cea6607) into master (97ecd53) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #144   +/-   ##
=======================================
  Coverage   93.55%   93.55%           
=======================================
  Files          24       24           
  Lines        3056     3059    +3     
=======================================
+ Hits         2859     2862    +3     
  Misses        197      197           
Files Changed Coverage Δ
src/apply.jl 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ba2tro
Copy link
Contributor

ba2tro commented Sep 18, 2023

This looks perfect! Looks like I also missed the fact that embed can permute the system for us given the indices instead of separately calling permutesystems. The issue with the previous version was that we were only calling embed when the basis for state and operator were not equal, but it also needed to be called when its equal but the order of indices isn't ascending.
I see that is_apply_shortcircuit fixes that.

@Krastanov Krastanov merged commit 4c6296c into qojulia:master Sep 18, 2023
@Krastanov Krastanov deleted the applyfixes branch September 18, 2023 16:16
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.

2 participants