Preserve solver statistics compatibility - #119
Conversation
| function Base.getproperty(solution::Solution, name::Symbol) | ||
| if name === :energies || name === :bond_dims || name === :elapsed_times | ||
| Base.depwarn( | ||
| "`solution.$name` is deprecated; use `solution.stats.$name` instead.", | ||
| name, | ||
| ) | ||
| return getproperty(getfield(solution, :stats), name) | ||
| end | ||
|
|
||
| return getfield(solution, name) | ||
| end | ||
|
|
||
| function Base.propertynames(::Solution, _private::Bool=false) | ||
| fields = fieldnames(Solution) | ||
| aliases = (:energies, :bond_dims, :elapsed_times) | ||
| return (fields..., aliases...) | ||
| end | ||
|
|
There was a problem hiding this comment.
The new stats interface will only appear in the next release together with other breaking-changes. We are still on the v0.* of the solver precisely because we are still experimenting and refining the API. Do you see any gain in keeping the old, clunkier way around?
| function Base.getproperty(solution::Solution, name::Symbol) | |
| if name === :energies || name === :bond_dims || name === :elapsed_times | |
| Base.depwarn( | |
| "`solution.$name` is deprecated; use `solution.stats.$name` instead.", | |
| name, | |
| ) | |
| return getproperty(getfield(solution, :stats), name) | |
| end | |
| return getfield(solution, name) | |
| end | |
| function Base.propertynames(::Solution, _private::Bool=false) | |
| fields = fieldnames(Solution) | |
| aliases = (:energies, :bond_dims, :elapsed_times) | |
| return (fields..., aliases...) | |
| end |
There was a problem hiding this comment.
Addressed in PR #112 commit b9de6e3. I removed the transitional getproperty/propertynames aliases and their documentation/tests, leaving solution.stats as the canonical interface for the next breaking release.
| @test !is_feasible(psi) | ||
| @test isempty(psi.stats.energies) | ||
| @test isempty(psi.stats.bond_dims) | ||
| @test isempty(psi.stats.elapsed_times) | ||
| @test isempty(psi.stats.variances) | ||
| @test length(psi.stats.max_bonds.projections) == 1 | ||
| @test all(>(0), psi.stats.max_bonds.projections) | ||
| @test psi.stats.max_bonds.objective > 0 | ||
| @test psi.stats.max_bonds.initial_state == 0 | ||
| @test psi.stats.max_bonds.hamiltonian > 0 | ||
| @test [0, 0] ∉ psi | ||
| @test_throws DomainError TenSolver.sample(psi) | ||
|
|
||
| io = IOBuffer() | ||
| E_debug, psi_debug = with_logger(ConsoleLogger(io, Logging.Debug)) do | ||
| minimize( | ||
| zeros(2, 2); | ||
| constraints=impossible, | ||
| verbosity=0, | ||
| ) | ||
| end | ||
| debug_log = String(take!(io)) | ||
| @test E_debug == Inf | ||
| @test !is_feasible(psi_debug) | ||
| @test occursin("empty feasible subspace", debug_log) | ||
| @test !occursin("Exception while generating log record", debug_log) |
There was a problem hiding this comment.
Considering #102, we should think before adding trivial tests.
For example, why are we checking if the bond dimensions are non-negative when it is so by construction?
There was a problem hiding this comment.
Addressed in PR #112 commit b9de6e3. I removed the positivity assertions and duplicate solver calls, retained the semantic infeasible/logging checks, and folded mixed variance-cadence coverage into the existing statistics solve.
2 participants
Summary
solution.stats.variances, usingnothingwhen an iteration did not run the configured variance checkon_iterationcallback signature and preserve the released top-level statistics properties as deprecated aliasesContext
This is a focused compatibility and correctness follow-up to #118. It also gives #112 a stable statistics API so its benchmark can read the final checked variance without installing a callback.
Validation
julia +1.12 --project=. --startup-file=no -e 'using Pkg; Pkg.test(; coverage=false)'julia +1.12 --project=docs/ --startup-file=no -e 'using Pkg; Pkg.develop(PackageSpec(path=pwd())); Pkg.instantiate()'julia +1.12 --project=docs/ --startup-file=no docs/make.jl localgit diff --checkRefs #118
Refs #112