Jutho / TensorKit.jl

A Julia package for large-scale tensor computations, with a hint of category theory
MIT License
234 stars 41 forks source link

Fix a bug with svd of AdjointTensorMap #24

Closed mhauru closed 4 years ago

mhauru commented 4 years ago

In the current master, observe the following:

julia> using TensorKit
julia> t = TensorMap(randn, Float64, ℂ^2 ← ℂ^2)'
AdjointTensorMap(ProductSpace(ℂ^2) ← ProductSpace(ℂ^2)):
 -0.44957311339859585  0.574946008374467 
 -1.0585667067396305   0.8048926380999573
julia> svd(t, (1,), (2,))
(0.0, AdjointTensorMap(ProductSpace(ℂ^2) ← ProductSpace(ℂ^2)):
 0.47443890076071277   0.8802884353693206 
 0.8802884353693206   -0.47443890076071277
, AdjointTensorMap(ProductSpace(ℂ^2) ← ProductSpace(ℂ^2)):
 1.508085327421287  0.0                
 0.0                0.16362510057556934
, AdjointTensorMap(ProductSpace(ℂ^2) ← ProductSpace(ℂ^2)):
 -0.7593330317060951  0.650702195293692
  0.650702195293692   0.759333031706095
)

The truncation error has been moved to being the first return value. Fixing this is of course a breaking change, but I really hope noone has code that does things like err, U, S, Vt = svd(t', indsout, indsin).

By the way, this only comes up when specifying the index lists. Just calling svd(t) incurs a copy(t), which removes the adjoint structure, and thus

julia> svd(t)
(TensorMap(ProductSpace(ℂ^2) ← ProductSpace(ℂ^2)):
 -0.47443890076071293  -0.8802884353693207 
 -0.8802884353693206    0.47443890076071293
, TensorMap(ProductSpace(ℂ^2) ← ProductSpace(ℂ^2)):
 1.5080853274212869  0.0                
 0.0                 0.16362510057556925
, TensorMap(ProductSpace(ℂ^2) ← ProductSpace(ℂ^2)):
  0.7593330317060952  -0.6507021952936921
 -0.6507021952936921  -0.7593330317060952
, 0.0)

I also included a test that catches this, by trying all the decomposition tests also on AdjointTensorMaps.

Jutho commented 4 years ago

Great change; thanks for catching this.

coveralls commented 4 years ago

Coverage Status

Coverage increased (+0.8%) to 75.167% when pulling 9cf64127a3fd59d3e098b68a3ccad4345937e678 on mhauru:master into b179ba71eb0abd2a7b41b809ed10019656d16919 on Jutho:master.

codecov[bot] commented 4 years ago

Codecov Report

Merging #24 into master will increase coverage by 0.82%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #24      +/-   ##
==========================================
+ Coverage   76.85%   77.68%   +0.82%     
==========================================
  Files          27       27              
  Lines        2528     2541      +13     
==========================================
+ Hits         1943     1974      +31     
+ Misses        585      567      -18
Impacted Files Coverage Δ
src/tensors/factorizations.jl 86.95% <100%> (+2.24%) :arrow_up:
src/tensors/abstracttensor.jl 72.58% <0%> (-5.06%) :arrow_down:
src/tensors/tensoroperations.jl 74.42% <0%> (-1.77%) :arrow_down:
src/tensors/tensor.jl 60.86% <0%> (ø) :arrow_up:
src/spaces/productspace.jl 51.94% <0%> (ø) :arrow_up:
src/sectors/sectors.jl 58.22% <0%> (ø) :arrow_up:
src/fusiontrees/iterator.jl 92.22% <0%> (+0.08%) :arrow_up:
src/sectors/product.jl 82.19% <0%> (+0.5%) :arrow_up:
src/tensors/linalg.jl 75.14% <0%> (+1.2%) :arrow_up:
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update e1bce2d...9cf6412. Read the comment docs.