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

Eliminate type parameters from abstract types and add fullbasis function #38

Closed
wants to merge 4 commits into from

Conversation

akirakyle
Copy link
Member

@akirakyle akirakyle commented Dec 5, 2024

Following on discussion in #26 and #34, this is the first step towards improving the api around basis, by eliminating the type parameters from the abstract quantum object types and adding fullbasis. This will require minimal changes to QuantumOpticsBase because the currently necessary type parameters can be reintroduced when subtyping the abstract types.

PS, this PR is already on top of #37.

Copy link

codecov bot commented Dec 5, 2024

Codecov Report

Attention: Patch coverage is 0% with 72 lines in your changes missing coverage. Please review.

Project coverage is 34.25%. Comparing base (f22adb7) to head (3fc123c).

Files with missing lines Patch % Lines
src/show.jl 0.00% 43 Missing ⚠️
src/julia_base.jl 0.00% 15 Missing ⚠️
src/linalg.jl 0.00% 5 Missing ⚠️
src/bases.jl 0.00% 3 Missing ⚠️
src/embed_permute.jl 0.00% 3 Missing ⚠️
src/julia_linalg.jl 0.00% 2 Missing ⚠️
src/identityoperator.jl 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main      #38       +/-   ##
===========================================
+ Coverage   17.08%   34.25%   +17.16%     
===========================================
  Files          12       12               
  Lines         398      400        +2     
===========================================
+ Hits           68      137       +69     
+ Misses        330      263       -67     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Krastanov
Copy link
Collaborator

Thanks! I will try to find some time to look into this work.

A minor nit: if you are moving around a lot of code without modifying it, it helps a lot to have the moving and the editing in separate commits -- the review is much faster as the reviewer has an easier time focusing on the actual edits without wasting time on verifying that code is simply moved. No need to do it here after the fact, just a heads up for next time.

@akirakyle
Copy link
Member Author

The reorganization is the first commit here, which is also alone in PR in #37, since I figured you would see that first after the review request, then I would request review here only after that had been merged. If this is too confusing, let me know whatever is easiest for you! I admit I'm not so adept at the github interface since I usually use magit in emacs 😅

@akirakyle akirakyle force-pushed the bases-api branch 2 times, most recently from bf5f454 to dd38fae Compare December 5, 2024 21:22
@akirakyle akirakyle changed the title Elimite type parameters from abstract types and add fullbasis function Eliminate type parameters from abstract types and add fullbasis function Dec 5, 2024
@akirakyle akirakyle marked this pull request as draft December 5, 2024 21:29
@akirakyle
Copy link
Member Author

Converting to a draft for now since I just realized methods in expect_variance.jl assume type parameters to AbstractOperator to check if the bases are equal and so is broken by removing the type parameters. That means removing type parameters will have to wait on resolving the interface in #34 for how we want to handle compatible basis checks.

@akirakyle
Copy link
Member Author

Closing in favor of #42

@akirakyle akirakyle closed this Dec 9, 2024
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