-
Notifications
You must be signed in to change notification settings - Fork 124
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
Remove size type parameter from ITensor and IndexSet [WIP] #591
Conversation
I was thinking of taking a swing at this one too, so much appreciated that you did! I'll look it over but let's definitely see Matt's comments. |
Benchmark resultJudge resultBenchmark Report for /home/runner/work/ITensors.jl/ITensors.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/ITensors.jl/ITensors.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Baseline resultBenchmark Report for /home/runner/work/ITensors.jl/ITensors.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Runtime information
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed that new behaviors replacing emptyITensor are a key point to discuss. Also important to check whether storage of IndexSet is now what we want, but looks good to me.
Codecov Report
@@ Coverage Diff @@
## master #591 +/- ##
==========================================
+ Coverage 80.17% 83.05% +2.88%
==========================================
Files 42 42
Lines 4494 4893 +399
==========================================
+ Hits 3603 4064 +461
+ Misses 891 829 -62
Continue to review full report at Codecov.
|
Thanks for the detailed review so far, @mtfishman! I'll address the comments as I find time, should be soon :) |
Benchmark resultJudge resultBenchmark Report for /home/runner/work/ITensors.jl/ITensors.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/ITensors.jl/ITensors.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Baseline resultBenchmark Report for /home/runner/work/ITensors.jl/ITensors.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Runtime information
|
I just did some timings of the JIT overhead of the master branch versus this branch. Running the 1d_heisenberg.jl DMRG example code, I find that from start to finish (all of the JIT time plus all five sweeps) the master branch takes 2min 20s while this PR branch takes 1min 20s. So a whole minute faster! Of course results will vary depending on the sample code and machine used. |
A bunch of these got detached from whatever they're meant to refer to... |
Ugh, pretty annoying. I added notes on which parts of the code the comments were referring to. |
Accidentally 😰 |
Ugh, forgot that I'd added a |
690679e
to
0594a29
Compare
Alright, fixed, and added back in the accidentally deleted method. |
It actually turned out to be an easy fix (it seems), as long as my solution is acceptable as a stopgap. |
Good! Yes I think that's a reasonable way to handle it, and I think it's fairly similar to how we handle that case in the C++ version. |
Benchmark resultJudge resultBenchmark Report for /home/runner/work/ITensors.jl/ITensors.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/ITensors.jl/ITensors.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Baseline resultBenchmark Report for /home/runner/work/ITensors.jl/ITensors.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Runtime information
|
OK, if this is otherwise good to go I think the only thing left to be dealt with is the doc build. |
Which I don't 100% understand, by the way, since it seems like the filtering mechanism isn't working? |
Yes, the output is not the most helpful there. What was going on is a minor change in the output of DMRG, so the filter had to be corrected. It was from a different PR where I think I merged it without fixing that doctest! But there was a second set of doctest errors too regarding doctests that used the ITensor{N} notation, so those had to be fixed too. Let's see if it passes now! It did locally for me. |
Incredible, thank you! Is this ready to go if the lights turn green? |
Benchmark resultJudge resultBenchmark Report for /home/runner/work/ITensors.jl/ITensors.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/ITensors.jl/ITensors.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Baseline resultBenchmark Report for /home/runner/work/ITensors.jl/ITensors.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Runtime information
|
It has my approval. Lets also get @mtfishman 's final signoff before merging, since he mentioned "remaining design issues" in his last comment. |
2f07e48
to
f49a064
Compare
Miles I should hopefully have kept your commit in even with the force push. |
Hm, looks like the Doc build is failing again. But it's easy to re-fix now that I know how to do it. Only a few lines to change back! |
Let me fix the HDF5 issue as well. Annoying to catch these when the tests take nearly an hour to run. |
Yes I do wish the tests ran more quickly. This PR will actually help with that compared to previous versions of the code! But we could certainly lighten some of the tests which have a combinatorial explosion of cases. I just pushed a commit which restores the ability of adding emptyITensor() to other tensors, using the code you had in the + and - function previously. The case of ITensor() + ITensor(i,j) errors - as it should - because the storage is not Empty and the dimensions don't match. Definitely good to have the HDF5 code back. I'll check the last commit to see if anything else we wanted got dropped. |
On my end it looks like the HDF5 writing fix for emptyITensor is still there. |
I haven't pushed it yet, one minute, just checking that it doesn't conflict with anything. Get Jim to buy you a 1000 core CI rig and then |
Benchmark resultJudge resultBenchmark Report for /home/runner/work/ITensors.jl/ITensors.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/ITensors.jl/ITensors.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Baseline resultBenchmark Report for /home/runner/work/ITensors.jl/ITensors.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Runtime information
|
Benchmark resultJudge resultBenchmark Report for /home/runner/work/ITensors.jl/ITensors.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/ITensors.jl/ITensors.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Baseline resultBenchmark Report for /home/runner/work/ITensors.jl/ITensors.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Runtime information
|
OK, good to go with a squash and merge if this passes? |
Well that was epic. Thanks again for getting this done! |
Thanks again for the reviews. Look forward to the coming feature film about this PR in theaters near you summer of 2021 😎 . |
I'll try to get ITensorsGPU.jl (which I've been neglecting) harmonized with this as well. |
Would close #584. Doesn't require any modifications to NDTensors.jl. Happy to change anything if the modifications (especially to
emptyITensor
) don't suit.