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

Fixes to alternating_update default printing #201

Merged
merged 5 commits into from
Sep 26, 2024
Merged

Conversation

emstoudenmire
Copy link
Contributor

This PR fixes an issue where the outputlevel keyword to the dmrg function does not produce any output. (This was noticed when calling it from the TNS solver code, just for some background context.) The changes include:

  • setting region_printer and sweep_printer to use the definitions in defaults.jl for both interfaces to alternating_update
  • including the inserter_kwargs into the all_kwargs named tuple in region_update so that parameters like cutoff will be available to the printers
  • making sure default_region_printer has defaults (of nothing) for the values cutoff, maxdim, and mindim to make that function robust (i.e. not to assume all of these will be passed, since the current pattern was crashing in cases where only, say, cutoff was passed into the inserter_kwargs)

src/solvers/defaults.jl Outdated Show resolved Hide resolved
@emstoudenmire
Copy link
Contributor Author

I tested the current version and it works well. I would have liked to make a unit test, and while I would know how for a custom printer, here it's the default printer so I couldn't think of a good way except for a kind of test that explicitly checks the stdout output.

@mtfishman
Copy link
Member

mtfishman commented Sep 26, 2024

Yeah, testing printing is tricky, since we don't want them to be too specific or else they are constantly breaking, and then we need to update tests any time we change how something prints.

Maybe you could add a test that checks that at least a certain number of lines are printed by default, and that the output contains certain strings we expect, like "cutoff", "maxdim", and "mindim"? That should be pretty robust and we shouldn't have to update it very often.

src/solvers/defaults.jl Outdated Show resolved Hide resolved
@emstoudenmire
Copy link
Contributor Author

The tests were working yesterday, except for the documentation, but now are failing.
Interestingly, I can reproduce the testing failure on my laptop, but only when I do activate . and > test within the REPL.
If I run the tests just by running runtests.jl things go better. So it's a bit unclear to me right now what's going on there.

One possible issue, though it may be a red herring, is that on my machine I can't install a testing dependency, "OrdinaryDiffEq" because of a version compatibility issue. (The Pkg error message is quite long and I don't know how to read it, unfortunately.)

@mtfishman
Copy link
Member

I remember seeing OrdinaryDiffEq cause subtle compatibility issues that I wasn't able to understand. I'm trying to run the tests again.

@mtfishman
Copy link
Member

mtfishman commented Sep 26, 2024

@emstoudenmire the Windows tests passed, maybe the tests are just timing out or failing for some other non-Julia-specific reason.

@emstoudenmire
Copy link
Contributor Author

I'm glad those passed. So then we can be pretty sure the code is ok. (The relevant tests were passing locally for me too, I was just having issues running the entire testing suite for some reason.)

Ready to merge?

@mtfishman mtfishman merged commit fc6fac2 into main Sep 26, 2024
3 of 6 checks passed
@mtfishman mtfishman deleted the fix_printer_defaults branch September 26, 2024 21:19
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.

2 participants