Use fewer allocations and more performant functions in runcircuit #304
Conversation
* Use `isodd`. It is more readable and more performant * Avoid allocations in a few places It's good practice to habitually code this way because: * The code is more concise, clean, and readable. * In cases where this is a bottleneck (probably not here) the increase in performance is very significant. * The code serves as model for future development so that the benefits above accrue.
Note: If the lines were wrapped. Or written one sentence per line, etc., the edits and diffs to the README would be easier to read. (But I think GH will highlight changed characters)
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## master #304 +/- ##
==========================================
- Coverage 87.67% 87.64% -0.03%
==========================================
Files 22 22
Lines 2491 2485 -6
==========================================
- Hits 2184 2178 -6
Misses 307 307
☔ View full report in Codecov by Sentry. |
| inds_sizes = vcat([[length(inds(g)) for g in layer] for layer in circuit_tensors]...) | ||
| end | ||
| noiseflag = any(x -> x % 2 == 1, inds_sizes) | ||
| layers = circuit_tensors isa Vector{<:ITensor} ? (circuit_tensors,) : circuit_tensors |
There was a problem hiding this comment.
Even better would be to split off into a function:
to_layers(ts::Vector{ITensor}) = [ts]
to_layers(ts::Vector{Vector{ITensor}}) = tsI'm weary of using a Tuple since our convention is that a layered circuit is nested layers of Vector, though I understand it avoids an allocation. I don't think we're worried about the performance of this part of the code though.
There was a problem hiding this comment.
That makes sense. I'm guessing that allocating here would change the efficiency of computing noise_flags very little if at all.
Would to_layers be more generally useful? You could use it elsewhere to support the convention a bit. If not, you might consider [circuit_tensors] : circuit_tensors.
In any case, part of the point of this PR is that code serves as an example. Even though using a Tuple doesn't affect the behavior here, someone could copy the code or idea to another location where your convention is expected to be followed. So using an Array makes sense.
Furthermore, there are plans to add more escape analysis to the Julia compiler in the not too distant future. I imagine the compiler will do [x] (x,) for you in simple cases like this.
There was a problem hiding this comment.
I think it's better to move into a function, I could imagine that would be useful in other places (and I could imagine expanding the functionality to convert other structures into our standard layered circuit format).
isodd. It is more readable and more performant.The performance of the code in question has been improved significantly. It was probably not a performance bottle neck. But it's as easy or easier to write non-allocating code (in this case). And the result is more readable (again in this case). So I think its good to promote it as a habit.
I did the following
An alternative is