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

Factoring out a common interface #73

Merged
merged 4 commits into from
Dec 10, 2022
Merged

Conversation

Krastanov
Copy link
Collaborator

related to qojulia/QuantumInterface.jl#1

Best to keep overall discussion in qojulia/QuantumInterface.jl#1 and postpone discussing this PR until a decision on the above issue is made.

Signed-off-by: Stefan Krastanov <[email protected]>
Signed-off-by: Stefan Krastanov <[email protected]>
Signed-off-by: Stefan Krastanov <[email protected]>
@codecov
Copy link

codecov bot commented Dec 4, 2022

Codecov Report

Merging #73 (7ab5d14) into master (57c25dd) will decrease coverage by 0.14%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master      #73      +/-   ##
==========================================
- Coverage   92.66%   92.52%   -0.15%     
==========================================
  Files          24       23       -1     
  Lines        3164     3064     -100     
==========================================
- Hits         2932     2835      -97     
+ Misses        232      229       -3     
Impacted Files Coverage Δ
src/fock.jl 98.66% <ø> (-0.09%) ⬇️
src/nlevel.jl 100.00% <ø> (ø)
src/operators.jl 96.29% <ø> (ø)
src/operators_lazysum.jl 95.28% <ø> (ø)
src/pauli.jl 95.34% <ø> (-0.26%) ⬇️
src/spin.jl 100.00% <ø> (ø)
src/spinors.jl 81.41% <ø> (ø)
src/states.jl 65.18% <ø> (ø)
src/superoperators.jl 81.30% <ø> (ø)
src/operators_dense.jl 92.03% <0.00%> (+0.19%) ⬆️

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

@Krastanov
Copy link
Collaborator Author

I think this is ready for a first-pass review. I will comment on a few points in the diffs

Copy link
Collaborator Author

@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.

@david-pl , in-line in this review I left a few comments explaining why various decisions were made

src/QuantumOpticsBase.jl Show resolved Hide resolved
src/bases.jl Show resolved Hide resolved
src/operators.jl Show resolved Hide resolved
src/states.jl Show resolved Hide resolved
@Krastanov Krastanov changed the title [WIP] Factoring out a common interface Factoring out a common interface Dec 4, 2022
@Krastanov
Copy link
Collaborator Author

Of note, the compat for QuantumInterface is set to the current LTS, julia 1.6, while the compat for QuantumOpticsBase is still at 1.3. I personally find it reasonable to push the compat to at least LTS.

@Krastanov
Copy link
Collaborator Author

friendly weekly bump on the QuantumInterface PR

having this in a published version would make it easier for me to work on QuantumSavory

@@ -16,6 +17,7 @@ UnsafeArrays = "c4a57d5a-5b31-53a6-b365-19f8c011fbd6"
Adapt = "1, 2, 3.3"
FFTW = "1.2"
LRUCache = "1"
QuantumInterface = "0.1.0"
Copy link
Member

Choose a reason for hiding this comment

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

Why not "0.1"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it means the same. 0.1.x implies [0.1.x, 0.2) and 0.1 implies [0.1.0, 0.2) which are the same for x=0. But maybe I misunderstood https://pkgdocs.julialang.org/v1/compatibility/

Copy link
Member

Choose a reason for hiding this comment

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

Oh, my mistake. Thanks for clarifying.

@david-pl
Copy link
Member

@Krastanov changes here look good to me (left one small comment regarding the compat). I'd now:

  • merge this PR
  • wait for the invalidations CI on the master branch
  • publish a new release

Thanks for the work!

@david-pl david-pl merged commit ee25d95 into qojulia:master Dec 10, 2022
@david-pl
Copy link
Member

@Krastanov CI with JET appears to be failing after the merge. Should I hold off releasing a new version?

@Krastanov
Copy link
Collaborator Author

@david-pl , I think this is fine. This CI would be failing relatively often because it is running on nightly and JET and julia nightly frequently get temporarily out of sync (JET uses some deep julia internals). If the error is "outside of tests" it is probably because of a nightly issue. If it is actually inside the tests, then there is something interesting to look for.

If this is too inconsistent, I am happy to remove JET from CI until I find a better way to keep them in sync?

@Krastanov
Copy link
Collaborator Author

Also, the reason this is a whole separate CI is so that it is not executed when Julia devs runs PkgEval. That way intermittent noise in the CI does not pollute the ecosystem CI.

@david-pl
Copy link
Member

@Krastanov thanks, just wanted to clarify this again to be sure. As far as I can tell it's a test failure, but due to a MethodError in Core.Compiler: https://github.com/qojulia/QuantumOpticsBase.jl/actions/runs/3665262051/jobs/6196216342#step:21:286

I created a new release so you should be good to go once the PR is merged.

@Krastanov
Copy link
Collaborator Author

Indeed, JET is calling various internal undocumented Julia compiler internals in order to do abstract interpretation. The author of JET is probably the main sources of changes to this part of julia, but every so often JET takes a bit to get updated to use the newly changed internals. Maybe it would be reasonable to simply not run it on nightly, however JET improves so fast right now, that I am happy to deal with the intermittent CI failures among my projects. Happy to adjust the CI to the QuantumOptics preferences though!

@Krastanov Krastanov deleted the quantumbase branch December 11, 2022 12:24
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