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

identityoperator(b) should produce SquareEye #111

Merged
merged 3 commits into from
Jun 23, 2023

Conversation

amilsted
Copy link
Collaborator

@amilsted amilsted commented Jun 22, 2023

With qojulia/QuantumInterface.jl#15, this allows us to produce identity operators that contain FillArrays.SquareEye again.

Worth noting that we could also just choose to do this by checking basis sizes. However, the way basis types are defined right now, this would require runtime dispatch. That said, maybe that's okay for constructing identity operators.

Edit: This is less important with #110. After that is merged, FillArrays.Eye ought to properly preserve sparsity.

@codecov
Copy link

codecov bot commented Jun 22, 2023

Codecov Report

Merging #111 (9e15e0c) into master (94ac58b) will increase coverage by 0.10%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #111      +/-   ##
==========================================
+ Coverage   93.34%   93.44%   +0.10%     
==========================================
  Files          24       24              
  Lines        2884     2898      +14     
==========================================
+ Hits         2692     2708      +16     
+ Misses        192      190       -2     
Impacted Files Coverage Δ
src/operators_sparse.jl 98.24% <100.00%> (+2.89%) ⬆️

... and 1 file with indirect coverage changes

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

@Krastanov
Copy link
Collaborator

This seems good to merge. I reran one of the tests after the merge of #110

I will merge, check for lingering errors and release. Then we can retest qojulia/QuantumInterface.jl#15 and release.

No need for new compats, right?

@Krastanov
Copy link
Collaborator

And just to confirm, this new function will not be dispatched on until qojulia/QuantumInterface.jl#15 is merged, right?

@Krastanov
Copy link
Collaborator

Actually, it seems cleaner to merge qojulia/QuantumInterface.jl#15 first as a breaking release and then bump the compat here. Feel free to proceed yourself with merging both of these, or let me know and I will be happy to do them today -- I will just wait on a confirmation in case I am missing something.

@amilsted
Copy link
Collaborator Author

amilsted commented Jun 22, 2023

I think this will overwrite the QI method. Indeed, the test have:

 │  WARNING: Method definition identityoperator(Type{T}, QuantumInterface.Basis) where {T<:Number} in module QuantumInterface at /home/runner/.julia/packages/QuantumInterface/20cdS/src/identityoperator.jl:22 overwritten in module QuantumOpticsBase at /home/runner/work/QuantumOpticsBase.jl/QuantumOpticsBase.jl/src/operators_sparse.jl:72.** incremental compilation may be fatally broken for this module **

So it's okay to release this first, but probably cleaner to first do a breaking QI release, as you say. I'm happy to let you do the honors ;)

@Krastanov Krastanov merged commit 13cb586 into qojulia:master Jun 23, 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