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: regression in identityoperator()+sparse_matrix due to the move to Eye #109 #110

Merged
merged 5 commits into from
Jun 22, 2023

Conversation

SatyaBade12
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Jun 22, 2023

Codecov Report

Merging #110 (b7e25fe) into master (94ac58b) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #110      +/-   ##
==========================================
+ Coverage   93.34%   93.37%   +0.02%     
==========================================
  Files          24       24              
  Lines        2884     2897      +13     
==========================================
+ Hits         2692     2705      +13     
  Misses        192      192              
Impacted Files Coverage Δ
src/operators_sparse.jl 96.42% <100.00%> (+1.07%) ⬆️

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

@Krastanov
Copy link
Collaborator

Thanks, @SatyaBade12 , the contribution is really appreciated! I added a few more methods through the webeditor, lets see if I broke anything! This should be mergeable shortly and I will then make an issue about possible further optimizations.

@amilsted
Copy link
Collaborator

amilsted commented Jun 22, 2023

Thanks @SatyaBade12 and @Krastanov. Should have tested these more thoroughly! I'm surprised sums and products of Eyes with numbers give us dense matrices. Arguably this should be done better in FillArrays, so perhaps we should file an upstream issue too.

Edit: JuliaArrays/FillArrays.jl#265

@amilsted
Copy link
Collaborator

amilsted commented Jun 22, 2023

It seems I probably missed this because SquareEye behaves differently to Eye under these operations. The former behaves correctly in sums and krons with sparse matrices, the latter does not.

identityoperator(some_basis) ought to be producing a SquareEye, but apparently it isn't. In any case, the general Eye case definitely needed fixing!

Edit: #111

@Krastanov
Copy link
Collaborator

@amilsted , I added tests for non-square operators. If this is green on CI I will merge it and we can then follow up with #111 (which would now have some broken tests)

@Krastanov Krastanov merged commit 0d3dfb4 into qojulia:master Jun 22, 2023
11 checks passed
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

3 participants