ITensorNetwork type fixes and other minor refactors. - #175
jack-dunham wants to merge 16 commits into
Conversation
|
Your PR no longer requires formatting changes. Thank you for your contribution! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #175 +/- ##
==========================================
+ Coverage 85.83% 86.68% +0.84%
==========================================
Files 15 15
Lines 685 706 +21
==========================================
+ Hits 588 612 +24
+ Misses 97 94 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
dee5559 to
faa56ff
Compare
| return tn | ||
| end | ||
|
|
||
| function supportof(tn::AbstractGraph, op::ITensorOperator) |
There was a problem hiding this comment.
I think maybe I'd call this operator_support, just to not make it sound so generic, i.e. we don't know what the "scope" of this function will be. Also, what about constraining to tn::AbstractITensorNetwork?
There was a problem hiding this comment.
I worry about constraining such functions as then they no longer "just work" on e.g. wrappers that do not subtype AbstractITensorNetwork.
As for the name, I have no strong opinions. I have been drafting the ITensorNetworkOperator object, where this function would also have a method for. I don't know if that changes your opinion (although operator_support would still work in that case).
There was a problem hiding this comment.
"wrappers that do not subtype AbstractITensorNetwork": We could always generalize when we have those cases (or maybe you have one already?). "I don't know if that changes your opinion": I think that still fits, i.e. I think operator_support would assume some structure of the operator, like it has input and output indices and you are specifically matching against input indices. We could always generalize the name as the use cases generalize.
There was a problem hiding this comment.
There is no specific case. It is more of a case of me deciding to choose the default to be AbstractGraph when it does not introduce type piracy or ambiguity after having to change this multiple times to accommodate QuotientView.
Happy to change the name.
| # PERF: fast lookup compared to `AbstractITensorNetwork` fallback. | ||
| dimnamevertices(tn::ITensorNetwork, name) = tn.dimname_vertices[name] | ||
| function dimnamevertices(tn::ITensorNetwork, name) | ||
| return get(tn.dimname_vertices, name, Set{vertextype(tn)}()) |
There was a problem hiding this comment.
I think this makes more sense, but was this inspired by a particular use case?
There was a problem hiding this comment.
It is for consistency with the fallback method (which returns an empty set).
…alse instead of erroring This is inline with the `Graphs` behaviour.
… not in dictionary This is now consistant with the fallback defn of `dimnamevertices`. would error previously.
… on a tensor network.
Avoids some minor code duplication.
Fix imports in `apply_operators.jl`
…patch Convention from `MatrixAlgebraKit`.
7225dab to
48534f9
Compare
|
|
||
| # (log|∏terms|, sign(∏terms)) | ||
| function sumlogabs(terms) | ||
| T = typeof(first(terms)) |
There was a problem hiding this comment.
I think eltype(terms) is better, since it would work when terms is empty.
There was a problem hiding this comment.
The problem is that real(eltype(terms)) fails if the eltype of terms is Any.
It might be better to just pass in T directly, and have a default if T <: Any.
| # give element type `Any`; collecting the values instead picks up the type they actually have. | ||
| function vertex_scalars(factors, messages, vertices) | ||
| return map(v -> vertex_scalar(factors, messages, v), vertices) | ||
| return [vertex_scalar(factors, messages, v) for v in vertices] |
There was a problem hiding this comment.
I think an issue with this version is that it now returns a Vector instead of a Dictionary, I think a Dictionary is a bit nicer since it preserves which scalar is associated with which vertex. Maybe we could do dictionary(vertices, [vertex_scalar(factors, messages, v) for v in vertices])?
There was a problem hiding this comment.
Ah, I see, I guess before it would have output whatever map infers from the input, so if vertices was a Vector it would output a Vector while if it was Indices it would output a Dictionary... A bit tricky to try to reproduce that behavior (which I think is kind of nice...). Maybe we could define our own narrow_map(f, x) function:
narrow_map(f, v) = map(f, v)
narrow_map(f, v::AbstactIndices) = dictionary(v, [f(x) for x in v])There was a problem hiding this comment.
The problem with map(f, v) is that the element type of the resulting collection ends up as Any. It is a consequence of the element type of ITensor not being parameterized. The comprehension automatically infers the element type at runtime, hence why I switched to that as it then means that sumlogabs gets a subtype of Number as its element type so real(T) works.
3 participants
Fixes and cleanups to the ITensorNetwork type, the BP message cache, and gate application.
add_edge!/rem_edge!on ITensorNetwork return false instead of throwing.dimnameverticesreturns an empty set for a name the network does not hold.operator_support(tn, op): the vertices carrying the operator's input index names. An absent name contributes no vertex; a name on several vertices throws.dimname_verticesand the underlying graph half-updated.finalize_substate!takes both subsolve and solve objects as arguments.bethe_free_energy's duplicated log-sum factored intosumlog.