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

Further development of Fock states #69

Merged
merged 16 commits into from
Aug 6, 2024
Merged

Conversation

apkille
Copy link
Member

@apkille apkille commented Jul 12, 2024

  • Adds a documentation page for using Fock states.
  • Moved predefined Fock objects from predefined.jl to a new file predefined_fock.jl.
  • Added displacement and phase shift operators, DisplaceOp and PhaseShiftOp, respectively.
  • Added simplification rules and rewriter with tests for Fock states.
  • Updated express functionality for Fock objects.

@apkille
Copy link
Member Author

apkille commented Jul 12, 2024

@Krastanov you might disagree with the following changes, which is OK and we can discuss them:

  • I replaced ContinuousCoherentState and DiscreteCoherentState with simply CoherentState, as that distinction didn't seem too important to include in this package, particularly since QuantumOptics.jl only has coherentstate.
  • In express_nolookup, before for Fock states we would change the dimension of the Fock basis to 2 with the warning that the cutoff wasn't defined, but of course this isn't always the case, as we can define a finite cutoff. So, if the cutoff is infinite, then we change the dimension to 2.

Copy link

codecov bot commented Jul 12, 2024

Codecov Report

Attention: Patch coverage is 96.15385% with 1 line in your changes missing coverage. Please review.

Project coverage is 74.65%. Comparing base (f291b77) to head (3f9fe6d).

Files Patch % Lines
src/QSymbolicsBase/predefined_fock.jl 93.75% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #69      +/-   ##
==========================================
+ Coverage   72.94%   74.65%   +1.70%     
==========================================
  Files          18       19       +1     
  Lines         791      789       -2     
==========================================
+ Hits          577      589      +12     
+ Misses        214      200      -14     

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

@Krastanov Krastanov self-requested a review July 17, 2024 20:04
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.

I have a bunch of questions related to some of the details of how numerical expressions happen. I think I prefer we follow different semantics for the cutoff. Let's consider these notes as a first round of discussion on the topic.

docs/src/fock.md Outdated Show resolved Hide resolved
docs/src/fock.md Outdated Show resolved Hide resolved
docs/src/fock.md Outdated Show resolved Hide resolved
docs/src/fock.md Outdated Show resolved Hide resolved
docs/src/fock.md Outdated Show resolved Hide resolved
ext/QuantumOpticsExt/QuantumOpticsExt.jl Outdated Show resolved Hide resolved
src/QSymbolicsBase/predefined_fock.jl Outdated Show resolved Hide resolved
src/QSymbolicsBase/predefined_fock.jl Outdated Show resolved Hide resolved
src/QSymbolicsBase/predefined_fock.jl Outdated Show resolved Hide resolved
src/QSymbolicsBase/predefined_fock.jl Show resolved Hide resolved
@apkille apkille requested a review from Krastanov July 23, 2024 21:53
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.

This is pretty good. I have the following requests:

  • bring back the warning on default cutoff
  • make the cutoff a keyword arg in the constructor (to make it easy to extend in the future)
  • remove one of the exports
  • check whether the naming change affects QuantumSavory and make sure the breaking change is mentioned in a changelong and an appropriate version bump is made

Feel free to merge without another review after this is done.

ext/QuantumOpticsExt/QuantumOpticsExt.jl Show resolved Hide resolved
src/QSymbolicsBase/QSymbolicsBase.jl Show resolved Hide resolved
src/QSymbolicsBase/express.jl Outdated Show resolved Hide resolved
ext/QuantumOpticsExt/QuantumOpticsExt.jl Show resolved Hide resolved
@Krastanov
Copy link
Member

let me know if I am missing something that is making the requests unreasonable

@apkille
Copy link
Member Author

apkille commented Aug 4, 2024

All of your requests sound good to me.

@Krastanov Edit: actually, there's one small issue related to the first request, which I mentioned earlier in the first round of review:

The problem with raising a warning in the constructor is that it creates allocations and increases the runtime. This causes tests in test_express_opt.jl to not pass. I instead added a warning to the docs page

@apkille apkille requested a review from Krastanov August 5, 2024 15:23
@Krastanov
Copy link
Member

ah, noted! Sounds good, let's keep it as a warning to the docs for now

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.

I agree to the plan about adding a warning to the docs.

@apkille
Copy link
Member Author

apkille commented Aug 6, 2024

@Krastanov copying this down below: in your commit you modified QuantumOpticsRepr so that it has optional arguments, yet in your review you said that it should have a keyword argument. Could you please clarify?

@Krastanov
Copy link
Member

My bad. I meant kwarg. Maybe with @kwdef

@apkille apkille dismissed Krastanov’s stale review August 6, 2024 22:05

permission to merge

@apkille apkille merged commit 80ce567 into QuantumSavory:main Aug 6, 2024
16 of 17 checks passed
@apkille apkille deleted the qo branch August 6, 2024 22:06
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