New indexing and iteration for ProductSector #141
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #141 +/- ##
=======================================
Coverage 79.94% 79.94%
=======================================
Files 42 42
Lines 4962 4962
=======================================
Hits 3967 3967
Misses 995 995 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
I just added some of my thoughts as a reply to the "can we make this faster" comment, but I am not sure if this really matters, and presumably that really is over-engineering things.
At the very least, I would say to merge as-is, and if necessary optimize later
| function Base.isless(p1::ProductSector{T}, p2::ProductSector{T}) where {T} | ||
| return isless(reverse(p1.sectors), reverse(p2.sectors)) | ||
| function Base.isless(p1::P, p2::P) where {P<:ProductSector} | ||
| return isless(findindex(values(P), p1), findindex(values(P), p2)) |
There was a problem hiding this comment.
I think here it is possible to try to avoid computing the manhattan index if we really want:
- first compute the manhattan distance and compare
- if equal, only the
localoffsetshould be compared
I would even guess that this second point can be determined without even computing the offset, by comparing the cartesianindices?
| @@ -13,3 +13,78 @@ function _kron(A, B) | |||
| end | |||
| return C | |||
| end | |||
|
|
|||
| # Manhattan based distance enumeration: I is supposed to be one-based index | |||
| # TODO: is there any way to make this faster? | |||
There was a problem hiding this comment.
There's definitely some number theory stuff that could come in handy here, if I am not mistaken, the relevant terms are triangular numbers, tetrahedral numbers, and their generalizations.
There are general formulas for this:
This is however for infinite grids, but I feel like it should be possible to split the grid into regions that are either hyperrectangular, or "hypertriangular", both of which have "volume formulas"?
There was a problem hiding this comment.
The binomial formula is what I started with, but that indeed only works if there are no upper bounds, i.e. if all the sectors involved are infinite.
|
|
||
| function Base.iterate(P::SectorValues{ProductSector{T}}, i=1) where {T<:SectorTuple} | ||
| Base.IteratorSize(P) != Base.IsInfinite() && i > length(P) && return nothing | ||
| return getindex(P, i), i + 1 |
There was a problem hiding this comment.
I guess here is the biggest possible speed trap, it feels like it could be possible to iterate through these without having to map to linear indices, similar to how cartesianindices can be iterated without the integer divisions.
There was a problem hiding this comment.
Given that iteration over all possible sector values is not really used in the code, except for making tests, I don't care too much about the speed of this :-)
|
Ok, I think I will merge as is. I have ran the tests locally, but currently the TensorKitSectors tests are not running in CI. While analysing CI, also noticed something weird. The MacOS tests are all slightly quicker then on Ubuntu (for the tensor tests, they do not have all the sector choices in common, but those that do were always a bit faster on Mac), up to the point of AD. The AD tests seem anyway to take very long compared to the rest of the tests on both platforms, but on Mac they become really excessive. The AD tests for |
|
It could also be the RNG, I think the AD tests are really fully random (including spaces/arrows etc) now, making it quite hard to control the weight of the tests... |
ProductSector can be indexed and will iterate using an order that is based on running through slices of constant Manhattan distance. This enables indexing and meaningful iteration if infinite sectors are included in the product sector.