QuantumSavory / QuantumSymbolics.jl

Computer algebra tools for symbolic manipulations in quantum mechanics and quantum information
MIT License
29 stars 9 forks source link

Further development of Fock states #69

Closed apkille closed 1 month ago

apkille commented 1 month ago
apkille commented 1 month ago

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

codecov[bot] commented 1 month ago

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 :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Krastanov commented 1 month ago

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

apkille commented 1 month ago

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

Krastanov commented 1 month ago

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

apkille commented 1 month ago

@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 commented 1 month ago

My bad. I meant kwarg. Maybe with @kwdef