Skip to content

Conversation

apkille
Copy link
Member

@apkille apkille commented May 11, 2025

I will add express functionalities for QuantumOptics and Gabs in separate PRs to make review easier. This review paper is a good quick reference for the quantum objects implemented here.

One thing to note: I would like to add symbolic position and momentum operators in the future. With the way the internals of QSymbolics are currently defined, this would require having support for PositionBasis and MomentumBasis types in QOBase.jl (https://github.com/qojulia/QuantumOpticsBase.jl/blob/c9c24f96ed9b79a2cb2eeeeb6fc497defedea6a5/src/particle.jl#L6-L63). I suppose we could migrate ParticleBasis subtypes into QuantumInterface.jl so that they could be used here? Would that be easy to do? Linking this issue qojulia/QuantumInterface.jl#33.

@apkille apkille requested a review from Krastanov May 11, 2025 07:59
Copy link

codecov bot commented May 11, 2025

Codecov Report

Attention: Patch coverage is 33.33333% with 6 lines in your changes missing coverage. Please review.

Project coverage is 75.96%. Comparing base (a4980ef) to head (16dff93).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/QSymbolicsBase/predefined_fock.jl 33.33% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #113      +/-   ##
==========================================
- Coverage   76.62%   75.96%   -0.67%     
==========================================
  Files          19       19              
  Lines         830      828       -2     
==========================================
- Hits          636      629       -7     
- Misses        194      199       +5     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@Krastanov Krastanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! A few comments below:

  • I noticed two separate pre-existing problems with the predefined_fock stuff. No need to fix them here, but could you make an issue for each to track them?
  • Some questions about how things are printed, how things are named, and a few type constraints.

@Krastanov
Copy link
Member

migrating ParticleBasis makes sense to me

@apkille
Copy link
Member Author

apkille commented May 12, 2025

@Krastanov let me know when this looks good to merge.

@apkille
Copy link
Member Author

apkille commented May 19, 2025

@Krastanov ping.

Copy link
Member

@Krastanov Krastanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the lateness on reviewing this. If the changes I made in the last commit make sense and are fine with you, and if the tests pass, feel free to merge.

@apkille
Copy link
Member Author

apkille commented Jun 4, 2025

LGTM. Failing tests are unrelated to this PR.

@apkille apkille merged commit 85aed5f into QuantumSavory:main Jun 4, 2025
16 of 20 checks passed
@apkille apkille deleted the fock branch June 4, 2025 20:25
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