Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NDTensors] In the effort to replace EmptyStorage with an empty DataType #1161

Closed
wants to merge 289 commits into from

Conversation

kmp5VT
Copy link
Collaborator

@kmp5VT kmp5VT commented Jul 26, 2023

Description

In general there can be problems with the EmptyStorage type as it does not act like the other storage types (i.e. store a datatype and a element type). This requires us to often solve corner cases. Replacing EmptyStorage with a Zeros for the datatype with understanding of what kind of datatype is being used (not simply FillArrays::Zeros). Additionally, use FillArrays::Diag as a replacement for uniformDiag number storage.

Checklist:

  • My code follows the style guidelines of this project. Please run using JuliaFormatter; format(".") in the base directory of the repository (~/.julia/dev/ITensors) to format your code according to our style guidelines.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added tests that verify the behavior of the changes I made.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • Any dependent changes have been merged and published in downstream modules.

@kmp5VT kmp5VT marked this pull request as draft July 26, 2023 21:49
@kmp5VT kmp5VT requested a review from mtfishman July 26, 2023 21:49
@kmp5VT kmp5VT changed the title [Refactor] In the effort to replace EmptyStorage with an empty DataType [Refactor] [NDTensors] In the effort to replace EmptyStorage with an empty DataType Jul 26, 2023
@kmp5VT
Copy link
Collaborator Author

kmp5VT commented Jul 31, 2023

@mtfishman I am having an issue here where code like ITensor(Index(2)) is failing because of

ERROR: Setting the type parameter of the type `NDTensors.Zeros{Float64, 1, Vector{Float64}}` at position `NDTensors.SetParameters.Position{1}()` to `Float64` is not currently defined. Either that type parameter position doesn't exist in the type, or `set_parameter` has not been overloaded for this type.
Stacktrace:
[1] error(s::String)
    @ Base ./error.jl:35
  [2] set_parameter(type::Type, position::NDTensors.SetParameters.Position{1}, parameter::Type)
    @ NDTensors.SetParameters ~/.julia/dev/ITensors/NDTensors/src/SetParameters/src/interface.jl:10
  [3] set_parameters(type::Type, position::NDTensors.SetParameters.Position{1}, parameter::Type)
    @ NDTensors.SetParameters ~/.julia/dev/ITensors/NDTensors/src/SetParameters/src/set_parameters.jl:24
  [4] set_eltype(arraytype::Type{NDTensors.Zeros{Float64, 1, Vector{Float64}}}, eltype::Type)
    @ NDTensors ~/.julia/dev/ITensors/NDTensors/src/abstractarray/set_types.jl:8
  [5] ITensor(as::NDTensors.AllowAlias, eltype::Type{Float64}, A::NDTensors.Zeros{Float64, 1, Vector{Float64}}, inds::Tuple{Index{Int64}}; kwargs::Base.Pairs{Symbol, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
    @ ITensors ~/.julia/dev/ITensors/src/itensor.jl:352
  [6] ITensor(as::NDTensors.AllowAlias, eltype::Type{Float64}, A::NDTensors.Zeros{Float64, 1, Vector{Float64}}, inds::Tuple{Index{Int64}})
    @ ITensors ~/.julia/dev/ITensors/src/itensor.jl:340
  [7] itensor(::Type, ::Vararg{Any}; kwargs::Base.Pairs{Symbol, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
    @ ITensors ~/.julia/dev/ITensors/src/itensor.jl:134
  [8] itensor(::Type, ::NDTensors.Zeros{Float64, 1, Vector{Float64}}, ::Vararg{Any})
    @ ITensors ~/.julia/dev/ITensors/src/itensor.jl:134
  [9] ITensor(ElT::Type{Float64}, is::Tuple{Index{Int64}})
    @ ITensors ~/.julia/dev/ITensors/src/itensor.jl:178
 [10] ITensor(ElT::Type{Float64}, is::Index{Int64})
    @ ITensors ~/.julia/dev/ITensors/src/itensor.jl:181
 [11] ITensor(is::Index{Int64})
    @ ITensors ~/.julia/dev/ITensors/src/itensor.jl:183
 [12] top-level scope
    @ REPL[8]:1

The code is ignoring my definition of set_parameter for the Zeros type. I am wondering if you can see if I am doing something wrong? Thanks!

@mtfishman
Copy link
Member

The definition of set_parameter looks ok. Importing the function may be a bit subtle since the SetParameter module is actually defined inside of NDTensors, maybe it is better to import it with a relative path, i.e. putting:

import .SetParameters: set_parameter

in imports.jl.

src/itensor.jl Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Patch coverage: 78.67% and project coverage change: -18.70% ⚠️

Comparison is base (f6c575b) 85.41% compared to head (ed284ab) 66.72%.
Report is 4 commits behind head on main.

❗ Current head ed284ab differs from pull request most recent head 2f38cef. Consider uploading reports for the commit 2f38cef 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    #1161       +/-   ##
===========================================
- Coverage   85.41%   66.72%   -18.70%     
===========================================
  Files          88       91        +3     
  Lines        8426     7885      -541     
===========================================
- Hits         7197     5261     -1936     
- Misses       1229     2624     +1395     
Files Changed Coverage Δ
src/ITensors.jl 100.00% <ø> (ø)
src/itensor/emptyitensor.jl 0.00% <0.00%> (ø)
src/mps/mpo.jl 24.09% <0.00%> (-72.73%) ⬇️
src/itensor/diagitensor.jl 74.00% <74.00%> (ø)
src/itensor/specialitensors.jl 75.00% <75.00%> (ø)
src/itensor/oneitensor.jl 78.37% <78.37%> (ø)
src/itensor/itensor.jl 80.00% <80.24%> (ø)
src/itensor/indexops.jl 80.76% <80.76%> (ø)
src/itensor/randomitensor.jl 85.36% <85.36%> (ø)
src/index.jl 79.48% <100.00%> (-1.02%) ⬇️
... and 2 more

... and 62 files with indirect coverage changes

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

@kmp5VT
Copy link
Collaborator Author

kmp5VT commented Sep 20, 2023

@mtfishman It looks like one test case that fails is related to the @strided mul! call in contract which matches the error on main. As a reminder per our conversation earlier; I am taking a break from this branch to work on the GPU code and when I return I will try to consolidate the changes here in to a more compact and NDTensors focused manner.

@mtfishman
Copy link
Member

Oh yeah, sorry about that. Fixing that should align with other things we talked about with making the contract code generic and efficient for both CPU and GPU operations. Thanks, that sounds like a good plan.

@kmp5VT
Copy link
Collaborator Author

kmp5VT commented Sep 20, 2023

@mtfishman not a problem, this was mainly just a note to remind both of us where I am in this PR next time we look at it!

@mtfishman
Copy link
Member

@kmp5VT can we close this? Will be superseded by #1213 and other PRs.

@mtfishman mtfishman changed the title [Refactor] [NDTensors] In the effort to replace EmptyStorage with an empty DataType [NDTensors] In the effort to replace EmptyStorage with an empty DataType Nov 1, 2023
@mtfishman mtfishman closed this Nov 1, 2023
@kmp5VT kmp5VT deleted the kmp5/refactor/fillarrays_redo branch April 15, 2024 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants