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

[ITensors] [BUG] readwrite test has unsafe file closure #1005

Closed
ryanlevy opened this issue Nov 9, 2022 · 4 comments · Fixed by #1007
Closed

[ITensors] [BUG] readwrite test has unsafe file closure #1005

ryanlevy opened this issue Nov 9, 2022 · 4 comments · Fixed by #1007
Labels
bug Something isn't working ITensors Issues or pull requests related to the `ITensors` package.

Comments

@ryanlevy
Copy link
Contributor

ryanlevy commented Nov 9, 2022

Description of bug

readwrite.jl tests have unsafe file closure behavior. Because of the manual file open and close, if an error occures close doesn't get called. Then anything else calling the file data.h5 will fail (on v0.3.20 and prior only).

Minimal runnable code

    fo = h5open("data.h5", "w")
    Δ = δ(i, i')
    cΔ = δ(ComplexF64, i, i')
    fo["delta_tensor"] = Δ
    fo["c_delta_tensor"] =close(fo)
Test Summary:       | Pass  Error  Total  Time
HDF5 Read and Write |    9      3     12  2.7s
  TagSet            |    1             1  0.7s
  Index             |    2             2  0.2s
  IndexSet          |    1             1  0.1s
  Dense ITensor     |    3             3  0.7s
  Diag ITensor      |           1      1  0.5s
  QN ITensor        |           1      1  0.2s
  MPO/MPS           |           1      1  0.1s
  DownwardCompat    |    2             2  0.1s

I have two suggestions for fixes: 1) the do file method outlined on the HDF5.jl readme or 2) include unique filenames for each file and remove them all at the end. My personal vote is 1) and I'm happy to open a PR either way

Suggested Fix

h5open("data.h5", "w") do fo
    Δ = δ(i, i')
    cΔ = δ(ComplexF64, i, i')
    fo["delta_tensor"] = Δ
    fo["c_delta_tensor"] =end
Test Summary:       | Pass  Error  Total  Time
HDF5 Read and Write |   13      1     14  0.5s
  TagSet            |    1             1  0.0s
  Index             |    2             2  0.0s
  IndexSet          |    1             1  0.0s
  Dense ITensor     |    3             3  0.0s
  Diag ITensor      |           1      1  0.3s
  QN ITensor        |    2             2  0.0s
  MPO/MPS           |    2             2  0.1s
  DownwardCompat    |    2             2  0.0s

Version information

  • Output from versioninfo():
julia> versioninfo()
Julia Version 1.8.1
Commit afb6c60d69a (2022-09-06 15:09 UTC)
Platform Info:
  OS: macOS (arm64-apple-darwin21.5.0)
  CPU: 10 × Apple M1 Max
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-13.0.1 (ORCJIT, apple-m1)
  Threads: 1 on 8 virtual cores
  • Output from using Pkg; Pkg.status("ITensors"):
julia> using Pkg; Pkg.status("ITensors")
Status `~/.julia/environments/v1.8/Project.toml`
  [9136182c] ITensors v0.3.20

(Thanks JanReimers on the forum for noticing this)

@ryanlevy ryanlevy added bug Something isn't working ITensors Issues or pull requests related to the `ITensors` package. labels Nov 9, 2022
@mtfishman
Copy link
Member

Thanks for the report @ryanlevy. I would also vote to change the tests to use the do-block syntax, a PR for that would be great!

We should also raise an issue about reading and writing delta tensors, if that hasn't already been raised as an issue.

@emstoudenmire
Copy link
Collaborator

Thanks agreed. I personally find the do syntax hard to read, but if it guarantees the proper close behavior then it's the way to go here.

Would also be good to know what the delta tensor issue is, thanks.

@ryanlevy
Copy link
Contributor Author

ryanlevy commented Nov 9, 2022

I think ITensors/NDTensors needs a new release for the diag HDF5 machinery to be included from 400a794. When I use the dev version (] dev NDTensors) all the tests pass
(PR incoming for the tests)

@mtfishman
Copy link
Member

Great, glad it is fixed on main. Thanks for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ITensors Issues or pull requests related to the `ITensors` package.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants