Add reusable testsuite for sector-dependent tests#473
Conversation
lkdvos
left a comment
There was a problem hiding this comment.
Just going to leave a first review and set of comments here, I have not yet looked at everything in an enormous amount of detail.
I want to start by saying I definitely like the general strategy of putting these tests into functions, I agree it's a good organizing principle and on top of that it makes it a lot easier to only run a subset of the tests in debugging workflows, thanks for getting this started!
I left some comments on the technicalities, although I think overall that looks good as well. I was also wondering if the TensorKitTestSuite.jl file should be in the testsuite/ folder or not?
I think the main thing that I was considering though is how to split up the various testsets, and how these end up being called.
Right now, you introduced both test_fusiontrees as a function to run all tests, but then have to manually loop over the test functions to be able to split up the double and single tree tests, so that might indicate it could be easier to simply have these as separate groups to begin with?
Another comment is that many of the tensor tests that are more or less independent end up in the same test function as well, is this a deliberate choice or just out of convenience?
A completely different thing that this PR made me think of is that currently, we are parallelizing over the different files in a testgroup, which is actually slightly suboptimal with respect to compilation times - it would probably be more beneficial to parallelize over the different symmetry types instead, since a lot of that compilation is then shared and not thrown away.
I'm not entirely sure (yet) if and how to achieve such a thing with the ParallelTestRunner, but it could be interesting to investigate how much that affects the CI times.
| :tensors => Dict{String, Expr}(), | ||
| ) | ||
|
|
||
| # cannot just esc() the body, because that would make it a closure, compile it at a fixed world age and break constprop=true |
There was a problem hiding this comment.
Do you have a sense of in how many places we are using the @constinferred, and how many of these could simply be @testinferred?
The latter don't have this problem I think, which would then avoid us having to deal with this complication, and if the amount of actual @constinferred uses are small, we can simply replace these by actual top-level functions with the consts filled in.
There was a problem hiding this comment.
I'll admit I don't truly understand how constant propagation works, but I have found some cases (perhaps all?) where we need to be careful with @testinferred compared to @constinferred. For example, there's insertleftunit/insertrightunit while explicitly passing a position. There's some @constprop and Val things happening there that I don't really understand, but for example:
V = ComplexSpace(2)
t = @testinferred rand(ComplexF64, V^2 ← (V^3)')
@testinferred insertrightunit(t, 2) # Evaluated: TensorMap{ComplexF64, ComplexSpace, 3, 3, Vector{ComplexF64}} != Union{TensorMap{ComplexF64, ComplexSpace, 3, 3, Vector{ComplexF64}}, TensorMap{ComplexF64, ComplexSpace, 2, 4, Vector{ComplexF64}}}
@testinferred insertrightunit(t, 2) constprop=true # no issue
@testinferred insertrightunit(t, Val(2)) constprop=false # no issue
@constinferred insertrightunit(t, 2) # no issuePassing Val(n) makes it work, but for example in split you can't pass Val(n), while in repartition you can again.
There was a problem hiding this comment.
The difference between @testinferred and @constinferred is mostly that @constinferred makes a small function that has literal values as baked-in constants, and verifies that this is inferred. For example:
@constinferred insertrightunit(t, 2)
# equivalent to:
temp_function(t) = insertrightunit(t, 2) # constant baked in!
@inferred temp_function(t)It does so by default for values that are literals - Char, Symbol, Int, and you can instruct it do capture other variables by adding $.
| using .SectorTestSuite: can_fuse, F_unitarity_test, R_unitarity_test | ||
| import .SectorTestSuite: random_fusion # TODO: is the method added below needed? | ||
|
|
||
| const testgroups = Dict{Symbol, Dict{String, Expr}}( |
There was a problem hiding this comment.
Both GPUArrays and TensorKitSectors use a regular untyped Dict(), is there a specific reason you changed this here?
There was a problem hiding this comment.
A habit of mine to type everything 🫠 I can remove this if it matters
There was a problem hiding this comment.
I'm fine with typing it, but the String and Symbol mix seems a bit strange to me 🙃
| testgroups[$group][$name] = $(QuoteNode(ex)) | ||
| $(esc(fn))(arg) = _run_testsuite_entry(testgroups[$group][$name], arg) |
There was a problem hiding this comment.
I'm slightly confused by having both a function and an expression stored, it might be simpler to just have a single access function that takes a group and name instead?
function run_testsuite(group, name, args...)
_run_testsuite_entry(testgroups[group][name], args...)
endThere was a problem hiding this comment.
I didn't think about this, but thinking about a benefit in hindsight, having the function and expression stored makes the different tests more discoverable. Typing something like TensorKitTestSuite.test_fusiontrees_<TAB> doesn't require you to know the string lookup.
Of course, your alternative will precisely avoid cluttering the namespace with similarly named functions. Thinking about it a bit more, your suggestion is in my opinion also easier to read, and removing the safe name allows for more flexible names for the tests. So I'll do this and we'll see how it looks.
There was a problem hiding this comment.
One thing to mention is that we don't actually have to do this in a testsuite-style like TensorKitSectors, if the goal is really to simply have the option to call some of these functions we can also just have a module that defines these tests as functions immediately, without needing a macro.
…ving file location
Downstream packages may not only want to test their custom
Sectorthrough TensorKitSectors' test suite, but also its compatibility within TensorKit directly.I made some design choices in this, none of which I'm really going to die on a hill for, so feel free to suggest changes.
random_fusionhas two different implementations though. I currently just added a method for the fusion tree tests, but perhaps we can make do with TKS' implementation.I'm not very good at probing how good or bad my approach to the macro was, so I left some comments which was my way of thinking out loud. In particular, the workaround to the constant propagation issue I had was presented by a robot friend, and I was just happy that it worked 🤣