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

Implement benchmarks for broadcasting QO types #411

Merged
merged 15 commits into from
Sep 18, 2024

Conversation

apkille
Copy link
Contributor

@apkille apkille commented Aug 11, 2024

Integrates very simple benchmarking into CI, specifically for bringing QO.jl up to speed with the SciML interface (#404). Note, at this current moment, this is not me reserving the following bounty #407. I am merely submitting this PR to benchmark work in merged PRs (#404, qojulia/QuantumOpticsBase.jl#172). There is still a substantial amount of work to be done in migrating other QO benchmarks to this suite.

I should also mention: to add support for solving SDEs with QO types, I defined the method wiener_randn! from DiffEqNoiseProcess.jl, which meant adding it as a dependency (this is a requirement for custom array types SciML/StochasticDiffEq.jl#579 (comment)).

@apkille
Copy link
Contributor Author

apkille commented Aug 11, 2024

Ah, StochasticDiffEq hasn't released a new version yet with my changes, so the SDE benchmarks currently fail.

Copy link

codecov bot commented Aug 11, 2024

Codecov Report

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

Project coverage is 96.65%. Comparing base (d2c71fe) to head (03c14e5).
Report is 14 commits behind head on master.

Files with missing lines Patch % Lines
src/stochastic_base.jl 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #411      +/-   ##
==========================================
- Coverage   97.03%   96.65%   -0.39%     
==========================================
  Files          18       19       +1     
  Lines        1554     1702     +148     
==========================================
+ Hits         1508     1645     +137     
- Misses         46       57      +11     

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

@apkille apkille marked this pull request as draft August 14, 2024 17:35
@apkille
Copy link
Contributor Author

apkille commented Aug 14, 2024

Converting to draft until SDE.jl bumps version, which should be soon as discussed on slack.

@david-pl david-pl marked this pull request as ready for review August 22, 2024 07:33
@david-pl david-pl marked this pull request as draft August 22, 2024 07:34
@apkille
Copy link
Contributor Author

apkille commented Aug 26, 2024

@Krastanov pinging for review. The performance tracking test is failing because the benchmark result of this branch cannot be compared with master. It should work for future PRs if we merge this. Correct me if I'm wrong.

@apkille apkille marked this pull request as ready for review August 26, 2024 08:36
@Krastanov Krastanov self-requested a review September 4, 2024 21:16
Copy link
Collaborator

@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 slow review. This is very valuable work, thank you! Do you mind making the following small changes?

@@ -104,3 +105,5 @@ function _check_const(op)
end
nothing
end

DiffEqNoiseProcess.wiener_randn!(rng::AbstractRNG,rand_vec::T) where {T<:Union{Bra,Ket,Operator}} = randn!(rng,rand_vec.data)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a bugfix for something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I had to define this method to enable SDEs to be solved on QO types. I didn't see an easy PR fix to submit to SciML so I defined it here. If this is too sketchy to you, let me know and I can do some more digging to work around this problem.

for prob in zip(("schroedinger", "master"), (:(bench_schroedinger), :(bench_master)))
name, bench = (prob[1], prob[2])
# benchmark solving ODE problems on data of QO types
SUITE[name]["pure"][string(dim)] = @benchmarkable solve(eval($bench)($dim; pure=true), DP5(); save_everystep=false)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am a bit worried that eval is part of the benchmark. Is there any change if instead you do solve(prob) setup=(prob=eval(...))?

Project.toml Outdated
@@ -34,7 +35,7 @@ OrdinaryDiffEq = "5, 6"
QuantumOpticsBase = "0.3, 0.4, 0.5"
RecursiveArrayTools = "2, 3"
Reexport = "0.2, 1.0"
StochasticDiffEq = "6"
StochasticDiffEq = "6, 6.68.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should probably just be one single entry 6.68 if you want to refuse 6.67

@@ -6,6 +6,7 @@ version = "1.1.1"
Arpack = "7d9fca2a-8960-54d3-9f78-7d1dccf2cb97"
DiffEqBase = "2b5f629d-d688-5b77-993f-72d75c75574e"
DiffEqCallbacks = "459566f4-90b8-5000-8ac3-15dfb0a30def"
DiffEqNoiseProcess = "77a26b50-5914-5dd7-bc55-306e6241c503"
Copy link
Collaborator

Choose a reason for hiding this comment

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

this requires a compat bound (I am surprised Aqua did not catch this... oh, we do not have Aqua in QuantumOptics.jl...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😅 I'll submit an issue...

prob_list = ("schroedinger", "master", "stochastic_schroedinger", "stochastic_master")
for prob in prob_list
SUITE[prob] = BenchmarkGroup([prob])
for type in ("pure", "custom")
Copy link
Collaborator

Choose a reason for hiding this comment

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

May we use "QO types" and "Base array types" instead of pure and custom? These confused me for a second

@apkille
Copy link
Contributor Author

apkille commented Sep 16, 2024

@Krastanov pinging for review.

@Krastanov Krastanov merged commit 373e89d into qojulia:master Sep 18, 2024
5 of 8 checks passed
@Krastanov
Copy link
Collaborator

Thank you! Great to finally have benchmarks as part of CI.

@apkille apkille deleted the benchmark branch September 19, 2024 10:37
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