saving EntanglementConsumer logs#377
Open
JoseRodriguezRomero wants to merge 16 commits intoQuantumSavory:masterfrom
Open
saving EntanglementConsumer logs#377JoseRodriguezRomero wants to merge 16 commits intoQuantumSavory:masterfrom
JoseRodriguezRomero wants to merge 16 commits intoQuantumSavory:masterfrom
Conversation
…nvenient file formats.
…into jose/consumer_logs_save
Contributor
Benchmark Results (Julia v1)Time benchmarks
Memory benchmarks
|
Krastanov
requested changes
Apr 6, 2026
Member
Krastanov
left a comment
Krastanov
left a comment
There was a problem hiding this comment.
Thanks for setting this up, it seems to be going in the right direction.
I have a few suggestions that should hopefully make the code simpler, shorter, and more local (not changing so many use locations). You can re-request review when they are done.
| NetworkNodeController, EndNodeController, LinkController | ||
| NetworkNodeController, EndNodeController, LinkController, | ||
| # save and load entanglement consumer logs | ||
| EntanglementConsumerLog, save_entanglement_consumer_log, load_entanglement_consumer_log |
Member
There was a problem hiding this comment.
let's avoid creating new public functions and structs -- they are a significant maintenance burden in the future
per-protocol log structure and saving/loading function also seems like a design that will not scale well when we add similar features to many other protocols
Author
There was a problem hiding this comment.
Reverted to how it was before. As of the writing of this comment, the test scripts still need to be reverted back (I will do this tomorrow).
The main issue with the current implementation is that there is no "simple" way to tell when the simulation has finally stopped, or at least I cannot easily see how, without having access to an exported save function to store the log in a file.
Author
There was a problem hiding this comment.
The test scripts have been reverted back to how they were before.
|
|
||
| $FIELDS | ||
| """ | ||
| @kwdef struct EntanglementConsumerLog |
Member
There was a problem hiding this comment.
Introducing this new type seems to have caused the need to change a lot of code in many other unrelated parts of the library. I would like to avoid breaking changes unless they are necessary. Admittedly this is only a "technically breaking", not "formally breaking" change as it is not touching public APIs, but it is easy to avoid it, so let's avoid it.
It also happens to be a change that introduce new formally public API end points -- avoiding that when possible also helps with the review and maintenance burden, so let's do it as well.
Lastly, the previous setup conformed to the Tables.jl AbstractRow API better that makes it more easy to consume from other packages, another reason I would prefer to stick to it.
Author
There was a problem hiding this comment.
Removed the new type.
| Gabs = "0eb812ee-a11f-4f5e-b8d4-bb8a44f06f50" | ||
| Graphs = "86223c79-3864-5bf0-83f7-82e725a168b6" | ||
| GraphsMatching = "c3af3a8c-b79e-4b01-bf44-c718d7e0e0d6" | ||
| HDF5 = "f67ccb44-e63f-5c2f-98bd-6dc0ccc4ba2f" |
Member
There was a problem hiding this comment.
this is a pretty significant dependency, let's keep it in a pkg extension
Author
There was a problem hiding this comment.
added extensions for HDF5 and CVS types.
| _save_log_hdf5(file_name, log) | ||
| elseif endswith(file_name, ".csv") | ||
| _save_log_csv(file_name, log) | ||
| elseif endswith(file_name, ".txt") |
Member
There was a problem hiding this comment.
txt is not a standard format, let's just remove it and keep only well defined standard formats
Author
There was a problem hiding this comment.
removed saving to .txt format.
|
|
||
| function _save_log_csv(file_name::String, log::EntanglementConsumerLog) | ||
| open(file_name, "w") do file | ||
| println(file, "time,obs1,obs2") |
Member
There was a problem hiding this comment.
use CSV.jl, do not hand-roll your own solution (put it in a pkg extension)
Author
There was a problem hiding this comment.
added support to saving to CSV as an extension using CSV.jl
| return | ||
| end | ||
|
|
||
| if endswith(file_name, ".h5") || endswith(file_name, ".hdf5") |
Member
There was a problem hiding this comment.
this type of switch statements (if/else sequences) are usually a sign that multiple dispatch should be used. Check the FileIO for a more central API for these that will be more scallable and extendable, with fewer if-else trees, e.g. see the save block here http://juliaio.github.io/FileIO.jl/stable/implementing/#All-at-once-I/O:-implementing-load-and-save
E.g. if HDF5 and CSV in julia are both built around the FileIO ecosystem (check if this is the case), then the proper solution would be to define a
FileIO.save(::Format{CSV}, ::EntanglementConsumer)
Author
There was a problem hiding this comment.
Added save functions as suggested.
Member
|
Let's also remove the metadata dict until we have an explicit use for it. It seems like something that might be more appropriate in the save function itself, not in the protocol. |
The current implementation is still incomplete since the loop inside `@resumable function (prot::EntanglementConsumer)()` never exits.
Member
|
I edited the title to not be the branch name -- the titles get auto-listed in release notes so it helps to have them be explanatory |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #377 +/- ##
==========================================
- Coverage 76.79% 76.29% -0.51%
==========================================
Files 53 57 +4
Lines 2504 2518 +14
==========================================
- Hits 1923 1921 -2
- Misses 581 597 +16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Krastanov
requested changes
Apr 24, 2026
Member
Krastanov
left a comment
Krastanov
left a comment
There was a problem hiding this comment.
some naming conventions and using hdf5 idiomatically seem to be the main outstanding issues
|
|
||
| import QuantumSavory | ||
| using QuantumSavory.ProtocolZoo | ||
| import FileIO, CSV, DataFrames |
Member
There was a problem hiding this comment.
dataframes is a big dependency, remove it if it can be removed
Comment on lines
+10
to
+11
| write(file, "obs1", [log.obs1 for log in prot._log]) | ||
| write(file, "obs2", [log.obs2 for log in prot._log]) |
Member
There was a problem hiding this comment.
need to figure out more standard names than obs1/obs2, so that it can be coordinated with the Sequence team
|
|
||
| function FileIO.save(save_file::FileIO.File{FileIO.DataFormat{:CSV},String}, prot::EntanglementConsumer; metadata::Union{Dict{String,Any},Nothing} = nothing) | ||
| if !isempty(prot._log) | ||
| CSV.write(save_file, DataFrames.DataFrame(prot._log, ["t", "obs1", "obs2"])) |
Member
There was a problem hiding this comment.
obs1 and obs2 should be better named
Member
There was a problem hiding this comment.
we should be idiomatic about hdf5
- it seems it has the notion of group and dataset, let's make sure we use those as appropriate
- it seems to have the notion of an attribute (for metadata) that would be more natural than the adhoc "metadata_keys"
- ``QuantumSavoryCSV ``no longer requires ``DataFrames`` as a dependency. - Both ``QuantumSavoryCSV`` and ``QuantumSavoryHDF5`` use the property names of ``_log`` in ``EntanglementConsumer`` instead of hard coded names.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Support for saving and loading log files of an
EntanglementConsumerprotocol instance. The logs can be saved in.txt,.csvand.h5formats. The saved data will consist of the two observables recorded by the EntanglementConsumer and the time of each event. Moreover, for.h5formatted files, an optional parameter has been added toto store any metadata a user may want to save (if the selected file format is
.txtor.csvthe metadata is not saved).