Observers.jl is broken by this package

[jaredjeya]

I've found that creating an observer (from the package Observers.jl) while ITensorInfiniteMPS is loaded causes an error. This is because on line 130 of the file ITensors.jl, you define a method for Base.copy:

ITensorInfiniteMPS.jl/src/ITensors.jl

Line 130 in cc42d94

Base.copy(is::IndexSet) = IndexSet(copy.(ITensors.data(is)))

However, in Observers.jl, the constructor creates a dataframe with columns initialised to Union{}[], and in the process of creating this dataframe, these columns are copied. Since T <: Union{} for all T, and since an IndexSet = Vector{Index}, technically Vector{Union{}} <: IndexSet and it uses the new definition of Base.copy. This immediately fails because, of course, this is not actually an index set and ITensors.data(is) is undefined. I think essentially what's happened here is accidental type piracy.

MWE

Below is a MWE showing that that creating an observer works fine until ITensorInfiniteMPS is loaded:

julia> using Observers

julia> obs = observer("foo" => (;) -> 1)
0×1 DataFrame
 Row │ foo      
     │ Union{} 
─────┴─────────

julia> using ITensors

julia> obs = observer("foo" => (;) -> 1)
0×1 DataFrame
 Row │ foo      
     │ Union{} 
─────┴─────────

julia> using ITensorMPS

julia> obs = observer("foo" => (;) -> 1)
0×1 DataFrame
 Row │ foo      
     │ Union{} 
─────┴─────────

julia> using ITensorInfiniteMPS

julia> obs = observer("foo" => (;) -> 1)
ERROR: MethodError: no method matching data(::Vector{Union{}})
The function `data` exists, but no method is defined for this combination of argument types.

Closest candidates are:
  data(::InfiniteCanonicalMPS)
   @ ITensorInfiniteMPS ...\ITensorInfiniteMPS\src\infinitemps.jl:35
  data(::ITensor)
   @ ITensors ...\ITensors\Zs2nC\src\itensor.jl:764
  data(::CelledVector)
   @ ITensorInfiniteMPS ...\ITensorInfiniteMPS\src\celledvectors.jl:78
  ...

Stacktrace:
  [1] copy(is::Vector{Union{}})
    @ ITensorInfiniteMPS ...\ITensorInfiniteMPS\src\ITensors.jl:130
  [2] _preprocess_column(col::Vector{Union{}}, len::Int64, copycols::Bool)
    @ DataFrames ...\DataFrames\kcA9R\src\dataframe\dataframe.jl:243
  [3] DataFrames.DataFrame(columns::Vector{Any}, colindex::DataFrames.Index; copycols::Bool)
    @ DataFrames ...\DataFrames\kcA9R\src\dataframe\dataframe.jl:227
  [4] DataFrame
    @ ...\DataFrames\kcA9R\src\dataframe\dataframe.jl:193 [inlined]
  [5] DataFrames.DataFrame(pairs::Vector{Pair{String, Vector{Union{}}}}; makeunique::Bool, copycols::Bool)
    @ DataFrames ...\DataFrames\kcA9R\src\dataframe\dataframe.jl:284
  [6] DataFrame
    @ ...\DataFrames\kcA9R\src\dataframe\dataframe.jl:274 [inlined]
  [7] observer(names::Vector{String}, functions::Vector{var"#7#8"}; kwargs::@Kwargs{})
    @ Observers ...\Observers\1ausc\src\observer.jl:9
  [8] observer
    @ ...\Observers\1ausc\src\observer.jl:8 [inlined]
  [9] observer(names::Tuple{String}, functions::Tuple{var"#7#8"}; kwargs::@Kwargs{})
    @ Observers ...\Observers\1ausc\src\observer.jl:17
 [10] observer
    @ ...\Observers\1ausc\src\observer.jl:16 [inlined]
 [11] #observer#19
    @ ...\Observers\1ausc\src\observer.jl:29 [inlined]
 [12] observer(name_function_pairs::Pair{String, var"#7#8"})
    @ ...\Observers\1ausc\src\observer.jl:28
 [13] top-level scope
    @ REPL[9]:1

NB: you can get the same stack trace with df = DataFrame("foo" => Union{}[])

Workarounds

1. Define a generic fallback method, ITensors.data(is) = is, so that Base.copy works correctly on the empty column
2. Construct your observer as observer(args; copycols=false), avoiding the use of Base.copy.

[mtfishman]

Thanks for the report. Those are some sketchy definitions, and anyway IndexSet was only being kept around for backwards compatibility and should be removed, so hopefully we can just delete those definitions.

[mtfishman]

Note that this would get fixed by #91, we'll look into getting that merged.

[mtfishman]

By the way, I didn't appreciate the fact that Array{Union{}} <: Array{<:T} for any type T, which would mean that overloading any Base function with Array{<:T}, even for a type T that you own, is a form of type piracy. That's a bit crazy and really constrains the kind of overloads one can/should make. The definition we have in the current code wasn't a good idea to begin with, but it makes me wonder about where else we might be doing something like that...

[mtfishman]

This should be fixed now that #91 is merged, let us know if you have any issues.