Skip to content

ITensorNetwork type fixes and other minor refactors. - #175

Open
jack-dunham wants to merge 16 commits into
mainfrom
jd/tensornetwork-type-updates
Open

jack-dunham wants to merge 16 commits into
mainfrom
jd/tensornetwork-type-updates

Conversation

@jack-dunham

@jack-dunham jack-dunham commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

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.
  • dimnamevertices returns an empty set for a name the network does not hold.
  • New 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.
  • Tensor assignment checks index names before mutating, so a rejected assignment no longer leaves dimname_vertices and the underlying graph half-updated.
  • finalize_substate! takes both subsolve and solve objects as arguments.
  • bethe_free_energy's duplicated log-sum factored into sumlog.

@ITensorBot

ITensorBot commented Sep 8, 2026

Copy link
Copy Markdown
Member

Your PR no longer requires formatting changes. Thank you for your contribution!

@codecov

codecov Bot commented Sep 8, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.41270% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 86.68%. Comparing base (5119963) to head (1ed3f96).

Files with missing lines Patch % Lines
src/tensornetwork.jl 96.42% 1 Missing ⚠️
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     
Flag Coverage Δ
docs 0.00% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jack-dunham
jack-dunham force-pushed the jd/tensornetwork-type-updates branch 2 times, most recently from dee5559 to faa56ff Compare September 9, 2026 18:12
Comment thread src/beliefpropagation/messagecache.jl Outdated
Comment thread src/abstracttensornetwork.jl Outdated
return tn
end

function supportof(tn::AbstractGraph, op::ITensorOperator)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Comment thread src/tensornetwork.jl Outdated
Comment thread src/tensornetwork.jl Outdated
Comment thread src/tensornetwork.jl
# 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)}())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this makes more sense, but was this inspired by a particular use case?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is for consistency with the fallback method (which returns an empty set).

@jack-dunham
jack-dunham force-pushed the jd/tensornetwork-type-updates branch from 7225dab to 48534f9 Compare September 14, 2026 18:56
Comment thread src/beliefpropagation/messagecache.jl Outdated

# (log|∏terms|, sign(∏terms))
function sumlogabs(terms)
T = typeof(first(terms))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think eltype(terms) is better, since it would work when terms is empty.

@jack-dunham jack-dunham Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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])?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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])

@jack-dunham jack-dunham Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

end

function operator_support(tn::AbstractGraph, op::ITensorOperator)
support = Set{vertextype(tn)}()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe use Dictionaries.Indices?

Comment thread src/tensornetwork.jl
function set!_tensornetwork(tn::ITensorNetwork, vertex, tensor)
newinds = dimnames(tensor)
function update_tensornetwork_metadata!(tn, vertex, tensor)
oldnames = isassigned(tn, vertex) ? dimnames(tn[vertex]) : Set()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
oldnames = isassigned(tn, vertex) ? dimnames(tn[vertex]) : Set()
oldnames = isassigned(tn, vertex) ? dimnames(tn[vertex]) : Set{dimnametype(tn)}()

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.

3 participants