[ITensors] Make `scalartype` accessible as `ITensors.scalartype` #1370

mtfishman · 2024-03-31T15:38:16Z

ITensorTDVP.jl appears to rely on the function scalartype (which is defined in NDTensors) being available as ITensors.scalartype.

It seems like that was dropped during the ITensorMPS reorganization, since now only ITensorMPS uses scalartype. This fixes that by adding using NDTensors: scalartype inside of the ITensors module, and adds a test that it is accessible as ITensors.scalartype.

I also made some namespace improvements in some tests.

EDIT: I also added a test file that tests what names are exported by ITensors, which I think is good practice to do from now on for any package/module. Before we were implicitly testing what got exported by typing using ITensors and then using a function in a test, which is both finicky and we are moving away from that import/using style.

mtfishman · 2024-03-31T15:42:43Z

[test ITensorMPS] [test ITensorTDVP] [test ITensorGaussianMPS]

emstoudenmire · 2024-03-31T15:48:32Z

Thanks... was just about to work on this

emstoudenmire · 2024-03-31T15:50:03Z

By the way, I meant to ask what @eval module $(gensym()) does in the testing files? I noticed that also in the ITensorTDVP tests and saw that you are adding it here as well.

github-actions · 2024-03-31T15:50:50Z

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

github-actions · 2024-03-31T15:51:46Z

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

codecov-commenter · 2024-03-31T16:01:56Z

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (main@ff938e2). Click here to learn what that means.

❗ Current head f3c75a3 differs from pull request most recent head ade0393. Consider uploading reports for the commit ade0393 to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1370   +/-   ##
=======================================
  Coverage        ?   50.41%           
=======================================
  Files           ?      113           
  Lines           ?     8963           
  Branches        ?        0           
=======================================
  Hits            ?     4519           
  Misses          ?     4444           
  Partials        ?        0

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

github-actions · 2024-03-31T16:02:20Z

Run ITensorTDVP tests from comment trigger: failed ❌
https://github.com/ITensor/ITensors.jl/actions/runs/8499270262

github-actions · 2024-03-31T16:09:12Z

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

github-actions · 2024-03-31T16:09:53Z

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

mtfishman · 2024-03-31T16:10:52Z

By the way, I meant to ask what @eval module $(gensym()) does in the testing files? I noticed that also in the ITensorTDVP tests and saw that you are adding it here as well.

It generates a module with a random name, i.e.:

julia> M1 = @eval module $(gensym())
       f() = 2
       end
Main.var"##226"

julia> M2 = @eval module $(gensym())
       f() = 3
       end
Main.var"##227"

julia> M1.f()
2

julia> M2.f()
3

I have found that it is better to wrap test files in modules, since it makes them more self contained, i.e. you can control exactly what goes into the namespace to be used by the tests in that test file. That makes sure you aren't accidentally using a variable or function that was defined in a previous test file (or even some other code you ran, if you are running the tests with include("runtests.jl")), and also makes sure that when you type something like include("runtests.jl") it doesn't pollute the namespace with temporary variables. On top of that, using gensym() is just a trick to make a random unique module name so we don't have to come up with new names for modules and potentially have clashes between module names.

Maybe it is better to make a small macro:

@temp_module begin
# Test code here
end

which internally is just defined as @eval module $(gensym()) to make the intent clearer.

This kind of thing (wrapping test files in modules) is pretty commonly suggested for avoiding potential namespace issues in tests:
https://github.com/YingboMa/SafeTestsets.jl
https://discourse.julialang.org/t/setup-and-teardown-in-test-module/68589
https://discourse.julialang.org/t/how-to-avoid-name-conflicts-in-mypackage-runtests-jl/57186
https://discourse.julialang.org/t/best-practices-for-julia-unit-testing/30858
https://discourse.julialang.org/t/my-unit-tests-are-interferring-with-each-other/21686

@eval module $(gensym()) is essentially how SafeTestsets.jl works, though that package introduces a new macro @safetestset that is meant to be a replacement for @testset as opposed to wrapping the entire file in a module, but I found that package had some issues with tests that involved macros.

emstoudenmire · 2024-03-31T16:14:48Z

Nice, thanks for explaining. I do think the idea of a macro would be a good one down the road to make it more 'self documenting'. But not a big priority of course.

mtfishman · 2024-03-31T16:16:36Z

For example:

julia> macro temp_module(expr)
         return :(@eval module $(gensym()) $expr end)
       end
@temp_module (macro with 2 methods)

julia> M = @temp_module begin
         f() = 2
       end
Main.var"##231"

julia> M.f()
2

Probably that is less opaque than directly using @eval module $(gensym()).

emstoudenmire · 2024-03-31T16:20:56Z

I do like that better. And perhaps even @temporary_module since it's not something that one would be typing too often.

mtfishman · 2024-03-31T16:23:27Z

Ah I see an issue with the current testing setup to test against ITensorTDVP. I think even though the workflow is checking out this PR, ITensorTDVP by default is configured to use the registered version of ITensors.jl instead of the dev version, so in practice it is still testing against the registered version v0.3.59 which doesn't have this change to scalartype.

I'm sure there is a way to fix that, probably in the workflow we have to activate the environment of ITensorTDVP and checkout ITensors for development, i.e. add something like:

using Pkg: Pkg
Pkg.activate("ITensorTDVP")
Pkg.develop("ITensors")

However, if that is run within the Github Actions workflow, I'm not certain if that will checkout the PR branch or the main branch. It may require inserting a full URL for the PR branch into Pkg.develop.

mtfishman · 2024-03-31T16:29:34Z

I see the tests also generate a lot of warnings like:

WARNING: Method definition update!(ITensors.ITensorMPS.AbstractObserver) in module ITensorMPS at /Users/mfishman/.julia/dev/ITensors/src/ITensorMPS/update_observer.jl:3 overwritten in module ITensorTDVP at /Users/mfishman/.julia/packages/ITensorTDVP/OmMNj/src/update_observer.jl:4.
ERROR: Method overwriting is not permitted during Module precompilation. Use `__precompile__(false)` to opt-out of precompilation.
WARNING: Method definition update!(ITensors.ITensorMPS.AbstractObserver) in module ITensorMPS at /Users/mfishman/.julia/dev/ITensors/src/ITensorMPS/update_observer.jl:3 overwritten in module ITensorTDVP at /Users/mfishman/.julia/packages/ITensorTDVP/OmMNj/src/update_observer.jl:4.
WARNING: Method definition kwcall(NamedTuple{names, T} where T<:Tuple where names, typeof(Observers.update!), ITensors.ITensorMPS.AbstractObserver) in module ITensorMPS at /Users/mfishman/.julia/dev/ITensors/src/ITensorMPS/update_observer.jl:3 overwritten in module ITensorTDVP at /Users/mfishman/.julia/packages/ITensorTDVP/OmMNj/src/update_observer.jl:4.
WARNING: Method definition contract(NDTensors.AlgorithmSelection.Algorithm{:fit, Kwargs} where Kwargs<:(NamedTuple{names, T} where T<:Tuple where names), ITensors.ITensorMPS.MPO, ITensors.ITensorMPS.MPS) in module ITensorMPS at /Users/mfishman/.julia/dev/ITensors/src/ITensorMPS/contract_mpo_mps.jl:15 overwritten in module ITensorTDVP at /Users/mfishman/.julia/packages/ITensorTDVP/OmMNj/src/contract_mpo_mps.jl:26.
WARNING: Method definition kwcall(NamedTuple{names, T} where T<:Tuple where names, typeof(NDTensors.contract), NDTensors.AlgorithmSelection.Algorithm{:fit, Kwargs} where Kwargs<:(NamedTuple{names, T} where T<:Tuple where names), ITensors.ITensorMPS.MPO, ITensors.ITensorMPS.MPS) in module ITensorMPS at /Users/mfishman/.julia/dev/ITensors/src/ITensorMPS/contract_mpo_mps.jl:15 overwritten in module ITensorTDVP at /Users/mfishman/.julia/packages/ITensorTDVP/OmMNj/src/contract_mpo_mps.jl:26.
WARNING: Method definition linsolve(ITensors.ITensorMPS.MPO, ITensors.ITensorMPS.MPS, ITensors.ITensorMPS.MPS) in module ITensorMPS at /Users/mfishman/.julia/dev/ITensors/src/ITensorMPS/linsolve.jl:25 overwritten in module ITensorTDVP at /Users/mfishman/.julia/packages/ITensorTDVP/OmMNj/src/linsolve.jl:26.
WARNING: Method definition linsolve(ITensors.ITensorMPS.MPO, ITensors.ITensorMPS.MPS, ITensors.ITensorMPS.MPS, Number) in module ITensorMPS at /Users/mfishman/.julia/dev/ITensors/src/ITensorMPS/linsolve.jl:25 overwritten in module ITensorTDVP at /Users/mfishman/.julia/packages/ITensorTDVP/OmMNj/src/linsolve.jl:26.
WARNING: Method definition linsolve(ITensors.ITensorMPS.MPO, ITensors.ITensorMPS.MPS, ITensors.ITensorMPS.MPS, Number, Number) in module ITensorMPS at /Users/mfishman/.julia/dev/ITensors/src/ITensorMPS/linsolve.jl:25 overwritten in module ITensorTDVP at /Users/mfishman/.julia/packages/ITensorTDVP/OmMNj/src/linsolve.jl:26.
WARNING: Method definition kwcall(NamedTuple{names, T} where T<:Tuple where names, typeof(KrylovKit.linsolve), ITensors.ITensorMPS.MPO, ITensors.ITensorMPS.MPS, ITensors.ITensorMPS.MPS) in module ITensorMPS at /Users/mfishman/.julia/dev/ITensors/src/ITensorMPS/linsolve.jl:25 overwritten in module ITensorTDVP at /Users/mfishman/.julia/packages/ITensorTDVP/OmMNj/src/linsolve.jl:26.
WARNING: Method definition kwcall(NamedTuple{names, T} where T<:Tuple where names, typeof(KrylovKit.linsolve), ITensors.ITensorMPS.MPO, ITensors.ITensorMPS.MPS, ITensors.ITensorMPS.MPS, Number) in module ITensorMPS at /Users/mfishman/.julia/dev/ITensors/src/ITensorMPS/linsolve.jl:25 overwritten in module ITensorTDVP at /Users/mfishman/.julia/packages/ITensorTDVP/OmMNj/src/linsolve.jl:26.
WARNING: Method definition kwcall(NamedTuple{names, T} where T<:Tuple where names, typeof(KrylovKit.linsolve), ITensors.ITensorMPS.MPO, ITensors.ITensorMPS.MPS, ITensors.ITensorMPS.MPS, Number, Number) in module ITensorMPS at /Users/mfishman/.julia/dev/ITensors/src/ITensorMPS/linsolve.jl:25 overwritten in module ITensorTDVP at /Users/mfishman/.julia/packages/ITensorTDVP/OmMNj/src/linsolve.jl:26.

We should fix those (say by just not overloading those functions from ITensors.jl) and include that along with this PR's fix in a new ITensors.jl v0.3.60, before we go ahead with ITensor/ITensorTDVP.jl#67, so that we have a "clean" starting point for proceeding with ITensor/ITensorTDVP.jl#67. Could you fix those overloading issues in a new PR?

mtfishman · 2024-03-31T16:34:53Z

I tested this fix locally and it fixes ITensorTDVP.jl so I will merge and register this (since it is pretty bad that ITensorTDVP.jl is broken with the latest version of ITensors.jl) and we can include the fixes to those overloading warnings in the next version. We can also investigate fixing the ITensorTDVP test workflow in future PRs.

mtfishman · 2024-03-31T16:54:10Z

@emstoudenmire a reference point for fixing the downstream testing of ITensorTDVP could be this workflow from BlockArrays: https://github.com/JuliaArrays/BlockArrays.jl/blob/v0.16.41/.github/workflows/downstream.yml#L49-L65

Some other references for testing a PR against dependencies are:
https://discourse.julialang.org/t/github-actions-workflow-for-reverse-dependency-integration-testing/53536
https://discourse.julialang.org/t/ann-github-action-workflow-to-test-if-your-pr-breaks-another-package-reverse-dependency-integration-testing/57635

mtfishman · 2024-03-31T16:56:09Z

Perhaps we can just use one of those instead. I think they don't support optionally running the test based on a comment trigger but it may be better to just run the test every time anyway.

emstoudenmire · 2024-03-31T21:34:45Z

Sounds good & I'll fix the warnings in another PR.

mtfishman added 2 commits March 31, 2024 11:32

[ITensors] Make accessible with

abd630f

Fix some test dependencies

ade0393

mtfishman changed the title ~~[ITensors] Make scalartype accessible with ITensors.scalartype~~ [ITensors] Make scalartype accessible as ITensors.scalartype Mar 31, 2024

mtfishman merged commit ce5203b into main Mar 31, 2024
10 of 12 checks passed

mtfishman deleted the ITensors_scalartype_namespace branch March 31, 2024 16:37

mtfishman mentioned this pull request Apr 3, 2024

[ITensorMPS] Observers package extension #1376

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ITensors] Make `scalartype` accessible as `ITensors.scalartype` #1370

[ITensors] Make `scalartype` accessible as `ITensors.scalartype` #1370

mtfishman commented Mar 31, 2024 •

edited

Loading

mtfishman commented Mar 31, 2024

emstoudenmire commented Mar 31, 2024

emstoudenmire commented Mar 31, 2024

github-actions bot commented Mar 31, 2024

github-actions bot commented Mar 31, 2024

codecov-commenter commented Mar 31, 2024 •

edited

Loading

github-actions bot commented Mar 31, 2024

github-actions bot commented Mar 31, 2024

github-actions bot commented Mar 31, 2024

mtfishman commented Mar 31, 2024 •

edited

Loading

emstoudenmire commented Mar 31, 2024

mtfishman commented Mar 31, 2024

emstoudenmire commented Mar 31, 2024

mtfishman commented Mar 31, 2024 •

edited

Loading

mtfishman commented Mar 31, 2024

mtfishman commented Mar 31, 2024

mtfishman commented Mar 31, 2024

mtfishman commented Mar 31, 2024

emstoudenmire commented Mar 31, 2024

[ITensors] Make scalartype accessible as ITensors.scalartype #1370

[ITensors] Make scalartype accessible as ITensors.scalartype #1370

Conversation

mtfishman commented Mar 31, 2024 • edited Loading

mtfishman commented Mar 31, 2024

emstoudenmire commented Mar 31, 2024

emstoudenmire commented Mar 31, 2024

github-actions bot commented Mar 31, 2024

github-actions bot commented Mar 31, 2024

codecov-commenter commented Mar 31, 2024 • edited Loading

Codecov Report

github-actions bot commented Mar 31, 2024

github-actions bot commented Mar 31, 2024

github-actions bot commented Mar 31, 2024

mtfishman commented Mar 31, 2024 • edited Loading

emstoudenmire commented Mar 31, 2024

mtfishman commented Mar 31, 2024

emstoudenmire commented Mar 31, 2024

mtfishman commented Mar 31, 2024 • edited Loading

mtfishman commented Mar 31, 2024

mtfishman commented Mar 31, 2024

mtfishman commented Mar 31, 2024

mtfishman commented Mar 31, 2024

emstoudenmire commented Mar 31, 2024

[ITensors] Make `scalartype` accessible as `ITensors.scalartype` #1370

[ITensors] Make `scalartype` accessible as `ITensors.scalartype` #1370

mtfishman commented Mar 31, 2024 •

edited

Loading

codecov-commenter commented Mar 31, 2024 •

edited

Loading

mtfishman commented Mar 31, 2024 •

edited

Loading

mtfishman commented Mar 31, 2024 •

edited

Loading