Store time of tagging #114
Conversation
ba2tro
added
the
Skip-Changelog
This reverts commit ad650ed.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #114 +/- ##
==========================================
+ Coverage 71.34% 71.40% +0.05%
==========================================
Files 37 37
Lines 1525 1528 +3
==========================================
+ Hits 1088 1091 +3
Misses 437 437 ☔ View full report in Codecov by Sentry. |
|
@Krastanov , any idea what the |
There was a problem hiding this comment.
I left some comments in. This seems to introduce an API change that breaks current code and requires the user to run a lot of extra boilerplate (carrying around the guid() calls). guid should not be a user-visible thing.
Not sure about why the documentation build fails. Do not worry about it for now.
src/networks.jl
Outdated
| @@ -14,6 +14,7 @@ struct RegisterNet | |||
| end | |||
|
|
|||
| function RegisterNet(graph::SimpleGraph, registers, vertex_metadata, edge_metadata, directed_edge_metadata) | |||
| glcnt[] = 0 # set the global counter of `guid`s to zero whenever a new network is initialized | |||
There was a problem hiding this comment.
someone might be modeling multiple networks. No need to set it to zero, to avoid clashes
There was a problem hiding this comment.
On the other hand, this seems pretty useful when it comes to tests... So maybe we should not have a global counter, rather a counter per network?
There was a problem hiding this comment.
Its straight forward to make the counter local to each network, but then taking in to account the previous comment about adding id and time automatically inside the tag!, the local id now would be a field of the network and we would need to pass the network to tag! in order to access it
There was a problem hiding this comment.
A new private field in the network makes sense, but having to pass around additional arguments is definitely not workable. The goal is not just to make something possible, equally important is for the interface to be something usable and logical.
Most of the objects that are being tagged carry a reference to the more centralized objects, e.g. the way get_time_tracker works. Maybe the best solution is to have get_time_tracker provide the object tracking the ids (e.g. by having a new simulation type).
Or you can just stick to a global non-zeroed counter, while fixing the rest of the things. That is fairly straightforward to solve separately.
There was a problem hiding this comment.
I think making net a field of our own Simulation type would be the best way about it then, as you suggested above. For now lets work with a the non-zeroed global counter, I'll open an issue for it
|
is this ready for review? |
|
Yes, sorry my bad, I should've re-requested for review |
|
no worries! You might have said so in person, but it helps to have it mentioned here as well in case I forget |
There was a problem hiding this comment.
This seems nearly ready.
I think we are still missing tests for the new functionality, though. Tests that previously would have failed, like requesting filo or fifo for a whole register where we have various slots of the register tagged in arbitrary order. Could you add such tests?
src/queries.jl
Outdated
| tags = ref.reg.tags[ref.idx] | ||
| i = findfirst(==(tag), tags) | ||
| isnothing(i) ? throw(KeyError(tag)) : deleteat!(tags, i) # TODO make sure there is a clear error message | ||
| function untag!(ref::RegRef, id::Int128) # TODO rather slow implementation. See issue #74 |
There was a problem hiding this comment.
Is the TODO comment still valid? What is necessary to fix it?
There was a problem hiding this comment.
I would consider it resolved, considering we're using a dictionary instead of a vector of tags, so deleting a tag just need the key to be dropped(deleted) rather than iterating and comparing for equality through each element of a vector of tags. If you agree I'll remove the TODO. This would also mean we could close #74
|
|
||
| @test queryall(reg, EntanglementCounterpart, 1, ❓) == [(slot = reg[i], depth = d, tag = Tag(EntanglementCounterpart,1,j+i)) for i in 2:4 for (d,j) in ((8,310),(5,110),(1,10))] | ||
| @test queryall(reg, EntanglementCounterpart, 1, ❓; filo=true) == [(slot = reg[i], depth = d, tag = Tag(EntanglementCounterpart,1,j+i)) for i in 2:4 for (d,j) in ((8,310),(5,110),(1,10))] | ||
| @test queryall(reg, EntanglementCounterpart, 1, ❓; filo=false) == [(slot = reg[i], depth = d, tag = Tag(EntanglementCounterpart,1,j+i)) for i in 2:4 for (d,j) in reverse(((8,310),(5,110),(1,10)))] |
There was a problem hiding this comment.
This test is very difficult to read. Tests should be obviously correct, not copy-pasting things that we generate by the code we are testing. This test is also fragile -- it will have to be modified if we switch how the id is represented.
I would suggest making a helper function in this file along the lines of strip_id so that you can do strip_id(querryall(...)) == ... in order to keep the test easy to read.
There was a problem hiding this comment.
It will be even easier to read than previously becaise we will not need the depth tag anywhere either.
Basically, remove id from these tests so that the tests are not fragile.
There was a problem hiding this comment.
these tests still look much more complicated than they used to
It looks to me like these tests that we already have are what you described? Let me know if I missed something here |
- the keyword was not actually used anywhere - message buffer queries are first-in-first-out, not filo
…test (note the need for a filter regex in the doctest)
|
I made some changes and polish. Please check each of the commits I have submitted in case something is off. Most of them have a clear description for why I added them. Those marked |
|
woohoo, this is in! |
Resolves #111