Skip to content

Conversation

@emstoudenmire
Copy link
Contributor

This PR fixes an issue caused during the recent refactor of the solvers iterators, related to handling of keyword arguments for the subspace expansion step. If the subspace_expand!_kwargs parameter was passed to eigsolve, and it didn't include another NamedTuple called eigen_kwargs within it, then the subspace expansion code would not obey the maxdim limit on the bond dimension.

This PR has the subspace code use the factorize_kwargs parameters to obtain the truncation parameters such as maxdim.

An alternative we could consider besides this PR would be to require a user to pass maxdim twice: once in factorize_kwargs and again in subspace_expand!_kwargs. But that would make the interface more complicated and factorize_kwargs is a fairly generic name, not tied to the specific extract!, update!, and insert! functions, it seems reasonable to use it in multiple places.

…g it take truncation parameters from factorize_kwargs.
@codecov
Copy link

codecov bot commented Dec 15, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.98%. Comparing base (6aad2cd) to head (d075f60).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #269   +/-   ##
=======================================
  Coverage   68.98%   68.98%           
=======================================
  Files          81       81           
  Lines        4024     4024           
=======================================
  Hits         2776     2776           
  Misses       1248     1248           
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 Sentry.
📢 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.

@mtfishman
Copy link
Member

mtfishman commented Dec 15, 2025

An alternative we could consider besides this PR would be to require a user to pass maxdim twice: once in factorize_kwargs and again in subspace_expand!_kwargs. But that would make the interface more complicated and factorize_kwargs is a fairly generic name, not tied to the specific extract!, update!, and insert! functions, it seems reasonable to use it in multiple places.

I would like it if there was an "expert level" mode where users can fully specify extract!_kwargs, update!_kwargs, and insert!_kwargs separely, where I guess one of those would have subspace_expand!_kwargs (to have full control over the arguments at different levels). I think it is reasonable that a user might want different cutoff values for subspace expansion and other truncations. But also it seems reasonable to use something like factorize_kwargs as a backup default value if subspace_expand!_kwargs isn't specified. I don't think we need to tackle that broader design here since I realize you are trying to solve a more narrow problem, but do you think this PR accommodates that broader design?

@mtfishman
Copy link
Member

So to be more concrete, the "expert level" I'm picturing might look like:

nsweeps = 5
nsites = 1
factorize_kwargs = (; cutoff = 1e-5, maxdim = 32)
extract!_kwargs = (; subspace_expansion!_alg = Algorithm"densitymatrix"(; factorize_kwargs = (; cutoff = 1e-4)))
dmrg(H, psi0; extract!_kwargs, factorize_kwargs, nsites, nsweeps, outputlevel)

where we could design it such that if subspace_expansion!_alg doesn't contain factorize_kwargs, it falls back to using the factorize_kwargs passed to dmrg.

Note that:

  1. I'm not suggesting we do that in this PR, I'm bringing that up because I find it easier to review PRs like this by considering how the design fits into a broader design plan (also if we don't consider a broader design, we might be iterating towards a local design minimum and then have to backtrack).
  2. I realize the syntax subspace_expansion!_alg isn't implemented right now, but I think that should be an expert-level syntax we should support for specifying the algorithm along with the keyword arguments of a certain function call.

@emstoudenmire
Copy link
Contributor Author

This is helpful feedback and what I was looking for. Fortunately there’s a workaround right now in the current interface, so this PR is definitely about making steps in the direction of a better one.

I’ll think about the above points and see if this PR is heading in the right direction and adjust if not.

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