Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ITensors] Add missing kwargs to replacebond! and downstream functions #1300

Merged

Conversation

brian-dellabetta
Copy link
Contributor

@brian-dellabetta brian-dellabetta commented Jan 3, 2024

Description

A previous PR to ITensors.jl (#1226) refactored away from kwargs... in function calls to replacebond!, factorize, and some downstream calls, but missed a few kwargs use_relative_cutoff, use_absolute_cutoff, and min_blockdim. This fix adds those back to the high level function and ensures they are passed down correctly into the eigen and svd decomposition routines.

Fixes #1297 (see issue for previous behavior)

How Has This Been Tested?

I updated the tests in test_mps.jl to include some of these keyword arguments in the replacebond! testset. We can add more to increase code coverage, but I wanted to get your thoughts on what I've modified so far before going any further.

  • test/ITensorLegacyMPS/base/test_mps.jl testset replacebond!

Checklist:

  • My code follows the style guidelines of this project. Please run using JuliaFormatter; format(".") in the base directory of the repository (~/.julia/dev/ITensors) to format your code according to our style guidelines.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added tests that verify the behavior of the changes I made.
  • [] I have made corresponding changes to the documentation. (no changes needed, just fixing a regression.)
  • My changes generate no new warnings.
  • Any dependent changes have been merged and published in downstream modules.

@brian-dellabetta brian-dellabetta changed the title add missing kwargs to mps replacebond! and downstream functions [ITensors] add missing kwargs to mps replacebond! and downstream functions Jan 3, 2024
@mtfishman
Copy link
Member

Thanks, looks good. The tests seem reasonable, though maybe we could leave the current tests as-is and just add a new minimal test that calls replacebond! with the keyword arguments you've added back.

@mtfishman
Copy link
Member

Note that a few tests were broken by the Julia 1.10 release which are being fixed in #1299, so unrelated to this PR.

@brian-dellabetta
Copy link
Contributor Author

Thanks @mtfishman , I split the calls to replacebond! with svd kwargs into a separate code block in the same testset. I'm not sure if there's some easy configuration that we could check to make sure the relative and absolute cutoffs are actually used, but that wasn't part of the previous test coverage so I held off for now. Lmk what you think. I can rebase after #1299 is merged in

@mtfishman
Copy link
Member

Looks good, thanks. I think it's sufficient for now just to check it accepts those keyword arguments, ideally those keyword arguments are tested in other parts of the test suite.

@mtfishman
Copy link
Member

Could you also pass min_blockdim to one of those replacebond! calls as well?

@brian-dellabetta
Copy link
Contributor Author

Could you also pass min_blockdim to one of those replacebond! calls as well?

Sure, updated. I thought it was only for blocksparse so I left it off originally

@mtfishman mtfishman changed the title [ITensors] add missing kwargs to mps replacebond! and downstream functions [ITensors] Add missing kwargs to replacebond! and downstream functions Jan 4, 2024
@mtfishman
Copy link
Member

[test ITensors mps]

Copy link
Contributor

github-actions bot commented Jan 4, 2024

Run ITensors mps tests from comment trigger: succeeded ✅
https://github.com/ITensor/ITensors.jl/actions/runs/7414584942

1 similar comment
Copy link
Contributor

github-actions bot commented Jan 4, 2024

Run ITensors mps tests from comment trigger: succeeded ✅
https://github.com/ITensor/ITensors.jl/actions/runs/7414584942

@mtfishman mtfishman merged commit 921f362 into ITensor:main Jan 4, 2024
8 of 9 checks passed
@brian-dellabetta brian-dellabetta deleted the bugfix/bd/mps-replacebond-kwargs branch January 4, 2024 21:26
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.

[ITensors] [BUG] Regression introduced to ITensors during refactor of NDTensors away from kwargs...
2 participants