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

Fix bug in ptranspose and added corresponding tests #98

Merged
merged 7 commits into from
May 30, 2023

Conversation

zejian-li
Copy link
Contributor

fixed bug reported in #97
the ptranspose(rho::DenseOpType{B,B}, indices::Union{Vector{Int},Tuple{Vararg{Int}}}) method is added to support multi-index subsystems, and the original single-index method ptranspose(rho::DenseOpType{B,B}, index::Int=1) is kept for compatibility, but implemented as a special case using the multi-index version.
tests dedicated to the two methods are added.

zejian-li added 4 commits May 18, 2023 17:18
corrected bug in ptranspose and added support for multi-index  subsystem in addition to the single-index method.
replaced tests for single-index ptranspose matching the corrected version; added tests for multi-index version of ptranspose
add type specification for `index::Int` argument
fix typo in code
@codecov
Copy link

codecov bot commented May 19, 2023

Codecov Report

Merging #98 (72bd2c3) into master (1953938) will decrease coverage by 3.63%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #98      +/-   ##
==========================================
- Coverage   96.65%   93.03%   -3.63%     
==========================================
  Files          26       24       -2     
  Lines        3410     3159     -251     
==========================================
- Hits         3296     2939     -357     
- Misses        114      220     +106     
Impacted Files Coverage Δ
src/metrics.jl 100.00% <100.00%> (ø)

... and 25 files with indirect coverage changes

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

add back original tests for ptranspose to keep code coverage
@david-pl
Copy link
Member

@zejian-li Thanks for this! It looks fine, I just have one small question: any reason you added another dispatch for index::Int ? In Julia, integers are iterable, so the method for ptranspose(rho::DenseOpType{B,B}, indices::Union{Vector{Int},Tuple{Vararg{Int}}}) also works for indices::Int if you just remove the typing, i.e.make the signature ptranspose(rho::DenseOpType{B,B}, indices). Then you can remove the additional ptranspose(rho::DenseOpType{B,B}, index::Int) dispatch altogether.

@zejian-li
Copy link
Contributor Author

@david-pl yes you are right! I will change the signature back to ptranspose(rho::DenseOpType{B,B}, indices=1), i.e. the original signature for the current implementation in the master branch, for compatibility.

zejian-li added 2 commits May 29, 2023 19:56
removing typing for second argument of `ptranspose` function
update docstring of ptranspose to be more general
@zejian-li
Copy link
Contributor Author

Should be good now. The negativity functions will also support now multiple indices without modification.

@david-pl david-pl merged commit ad17a1a into qojulia:master May 30, 2023
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