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

Added QuantumChannel #53

Merged
merged 14 commits into from
Nov 9, 2023
Merged

Added QuantumChannel #53

merged 14 commits into from
Nov 9, 2023

Conversation

ba2tro
Copy link
Member

@ba2tro ba2tro commented Oct 31, 2023

Added QuantumChannel using DelayQueue from ConcurrentSim.jl.

src/quantumchannel.jl Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 31, 2023

Codecov Report

Merging #53 (eeb78fb) into master (60d60e2) will increase coverage by 2.60%.
Report is 1 commits behind head on master.
The diff coverage is 100.00%.

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

@@            Coverage Diff             @@
##           master      #53      +/-   ##
==========================================
+ Coverage   63.29%   65.90%   +2.60%     
==========================================
  Files          27       28       +1     
  Lines        1079     1100      +21     
==========================================
+ Hits          683      725      +42     
+ Misses        396      375      -21     
Files Coverage Δ
src/QuantumSavory.jl 60.46% <ø> (+0.77%) ⬆️
src/concurrentsim.jl 46.42% <ø> (+9.84%) ⬆️
src/quantumchannel.jl 100.00% <100.00%> (ø)

... and 6 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

@Krastanov
Copy link
Member

Looking at the examples in ConcurrentSim, I do not think that take! and put! should be @resumable: https://github.com/JuliaDynamics/ConcurrentSim.jl/blob/8591bff85edb62556be142f141dafa506e0f7fae/src/resources/delayed_stores.jl#L56

src/quantumchannel.jl Outdated Show resolved Hide resolved
src/quantumchannel.jl Outdated Show resolved Hide resolved
src/QuantumSavory.jl Outdated Show resolved Hide resolved
src/quantumchannel.jl Outdated Show resolved Hide resolved
src/quantumchannel.jl Outdated Show resolved Hide resolved
src/quantumchannel.jl Outdated Show resolved Hide resolved
src/quantumchannel.jl Outdated Show resolved Hide resolved
@Krastanov
Copy link
Member

Thank you for all of this! I left a bunch of comments in. Some of them are fairly nit-picky -- it is so that we can establish some coding style conventions that might slowly become actual APIs. My nit-pickiness will diminish in future pull requests.

@ba2tro
Copy link
Member Author

ba2tro commented Nov 3, 2023

Please feel free to be as detailed with the reviews as you'd like, it can only help with improving the code quality of the repo, besides helping me code better ; )

@Krastanov
Copy link
Member

As you make changes, could you click "resolve" on the comments that are already dealt with. It makes it a bit easier to read through.

src/quantumchannel.jl Outdated Show resolved Hide resolved
src/quantumchannel.jl Outdated Show resolved Hide resolved
src/quantumchannel.jl Outdated Show resolved Hide resolved
src/quantumchannel.jl Outdated Show resolved Hide resolved
end

channel_reg = Register(qc)
swap!(rref, channel_reg[1])
Copy link
Member

Choose a reason for hiding this comment

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

does channel_reg has its time value correctly set after the swap!?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean the accesstimes attribute? No, it only swaps the staterefs and stateindices of the RegRefs, should we edit the current swap! to do so?

src/quantumchannel.jl Outdated Show resolved Hide resolved
swap!(rref, channel_reg[1])

if !isnothing(qc.background)
uptotime!(channel_reg[1], qc.queue.delay)
Copy link
Member

Choose a reason for hiding this comment

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

If swap! properly updates the time value, then this should be qc.queue.delay+now(qc.queue.store.env)

if isassigned(rref)
error("A take! operation is being performed on a QuantumChannel in order to swap the state into a Register, but the target register slot is not empty (it is already initialized).")
end
swap!(channel_reg[1], rref)
Copy link
Member

Choose a reason for hiding this comment

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

same question about swap! properly updating time here?

@Krastanov
Copy link
Member

I was unhappy with how cumbersome it was to use swap! so I made a few improvements there.

I edited the docstring a bit: (1) to remove implementation details and focus on the "why" (2) I think one of the doctests would have failed because it contained an arbitrary memory address.

But (2) also showed me that some tests are not running, so I double checked a few things there... We have not been checking doctests for a while. Lets make buildkite a priority.

@Krastanov Krastanov merged commit bf5c68d into QuantumSavory:master Nov 9, 2023
3 of 7 checks passed
@ba2tro ba2tro deleted the q_channel branch March 30, 2024 20:09
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