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

CorrelationFunctions with IndexedOperators #164

Merged
merged 3 commits into from
May 8, 2023

Conversation

j-moser
Copy link
Contributor

@j-moser j-moser commented Mar 28, 2023

  • added possibility to create a CorrelationFunction with IndexedOperators as op1 and op2
  • added possbility to evaluate CorrelationFunction
  • added a dispatch for MTK.ODESystem for indexed equations, since conjugate substituation is different
  • a CorrelationFunction can now be evaluated using a specified NumberedOperator by calling evaluate with additional number parameters
  • added test cases for the above

This update should also solve the issues 157,160 and 161

@codecov
Copy link

codecov bot commented Mar 29, 2023

Codecov Report

Merging #164 (109e1ca) into master (546ca6d) will increase coverage by 0.41%.
The diff coverage is 80.88%.

❗ Current head 109e1ca differs from pull request most recent head a00822f. Consider uploading reports for the commit a00822f to get more accurate results

@@            Coverage Diff             @@
##           master     #164      +/-   ##
==========================================
+ Coverage   81.05%   81.47%   +0.41%     
==========================================
  Files          24       24              
  Lines        4445     4598     +153     
==========================================
+ Hits         3603     3746     +143     
- Misses        842      852      +10     
Impacted Files Coverage Δ
src/QuantumCumulants.jl 100.00% <ø> (ø)
src/utils.jl 90.34% <0.00%> (-6.00%) ⬇️
src/index_average.jl 82.49% <71.42%> (+0.19%) ⬆️
src/index_correlation.jl 83.61% <78.66%> (+0.47%) ⬆️
src/index_utils.jl 94.82% <95.00%> (+1.49%) ⬆️
src/correlation.jl 88.82% <100.00%> (+1.56%) ⬆️
src/diffeq.jl 100.00% <100.00%> (ø)
src/equations.jl 96.49% <100.00%> (+0.41%) ⬆️
src/index_scale.jl 66.99% <100.00%> (+6.14%) ⬆️
src/qnumber.jl 90.25% <100.00%> (ø)

... and 1 file with indirect coverage changes

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

@tnadolny
Copy link

Hi, I was using the code from this pull request to do some things that did not work in the master branch. I thought I leave some comments, hoping they help to improve the package.

  • in test_indexed_correlation.jl, line 133: corr2_sc = scale(corr); Is this a typo? Should it be corr2_sc = scale(corr2);? Using the latter leads to an error in u0_c2 = correlation_u0(corr2_sc, sol.u[end]). This error is avoided when adding filter_func = phase_invariant to the definition of corr2 (this is needed, since eqs_c are also built with the filter function, right?).

  • I suggest to add a test including

S2_sc = Spectrum(corr2_sc)
ω = [-10:0.01:10;]Γ_
S2_sc(ω,sol.u[end],p0)
  • I still encounter problems with the function correlation_u0. I believe the elements in lhs = [average(substitute(op, subs)) for op in ops] are not always ordered properly, and therefore I sometimes get the error("Could not find initial value for $l !")

  • A similar problem occurs when I want to compute a spectrum using S(ω,sol.u[end],p0) and there are two different transitions involved, e.g.

hc = FockSpace(:cavity)
hA = NLevelSpace(:atomA,2)
hB = NLevelSpace(:atomB,2)
h = hc  hA  hB

When calling S(ω,sol.u[end],p0), I get an error UndefVarError: ⟨σᴮ121*σᴬ211⟩ not defined. I can imagine that the other order (⟨σᴬ211*σᴮ121⟩) would be defined. Is it possible that somewhere in generating the functions for the Spectrum, one should reorder according to the order of Hilbert spaces?

Thanks!

… getting averages in sums would not return a vector; added a test case for Spectrum and Correlation Functions
@ChristophHotter ChristophHotter merged commit 8fa4ac7 into qojulia:master May 8, 2023
2 checks passed
@ChristophHotter
Copy link
Member

Hi @tnadolny
Thank you for your comments. The new version v0.2.18 includes this PR from @j-moser.
I think all of the above comments are addressed and the issues #157 , #160 and #161 are solved in the new version.

@tnadolny
Copy link

tnadolny commented May 9, 2023

Yes, the three issues are indeed solved. Thank you!

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