Show improvements + subblocks iterator
#304
Conversation
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
You are keeping us in suspense ... I very much like the tensor printing improvements. The space printing I have no strong opinions about; I don't think the new scheme satisfies the "uses less space" criterion, but if this is more readable then I am definitely ok with this. I liked the aspect that copy-paste of standard REPL output yielded valid code, but I don't know how much I really rely on this. |
|
I'm also the least hung up on the graded spaces to be honest. Note that the main point was that the default printing truncates overly long outputs, and scales this with your current screen size, not necessarily that this is now vertical instead of horizontal. The REPL output being parse-able is still something that we have, you just have to call the default julia> show(V)
Rep[U₁](1 => 1, 2 => 1, 3 => 1, 4 => 1, 5 => 1, 6 => 1, 7 => 1, 8 => 1, 9 => 1, 10 => 1, 11 => 1, 12 => 1, 13 => 1, 14 => 1, 15 => 1, 16 => 1, 17 => 1, 18 => 1, 19 => 1, 20 => 1, 21 => 1, 22 => 1, 23 => 1, 24 => 1, 25 => 1, 26 => 1, 27 => 1, 28 => 1, 29 => 1, 30 => 1, 31 => 1, 32 => 1, 33 => 1, 34 => 1, 35 => 1, 36 => 1, 37 => 1, 38 => 1, 39 => 1, 40 => 1, 41 => 1, 42 => 1, 43 => 1, 44 => 1, 45 => 1, 46 => 1, 47 => 1, 48 => 1, 49 => 1, 50 => 1, 51 => 1, 52 => 1, 53 => 1, 54 => 1, 55 => 1, 56 => 1, 57 => 1, 58 => 1, 59 => 1, 60 => 1, 61 => 1, 62 => 1, 63 => 1, 64 => 1, 65 => 1, 66 => 1, 67 => 1, 68 => 1, 69 => 1, 70 => 1, 71 => 1, 72 => 1, 73 => 1, 74 => 1, 75 => 1, 76 => 1, 77 => 1, 78 => 1, 79 => 1, 80 => 1, 81 => 1, 82 => 1, 83 => 1, 84 => 1, 85 => 1, 86 => 1, 87 => 1, 88 => 1, 89 => 1, 90 => 1, 91 => 1, 92 => 1, 93 => 1, 94 => 1, 95 => 1, 96 => 1, 97 => 1, 98 => 1, 99 => 1, 100 => 1)I'm happy to remove the vertical printing and just do the horizontal truncated outputs instead, if that makes more sense? I would like to keep the little summary at the top though, the |
|
So, I now additionally added the julia> t = rand(V, V)
10←10 TensorMap{Float64, Rep[U₁], 1, 1, Vector{Float64}}:
Rep[U₁](0 => 1, 1 => 1, 2 => 1, 3 => 3, 4 => 1, 5 => 3) ← Rep[U₁](0 => 1, 1 => 1, 2 => 1, 3 => 3, 4 => 1, 5 => 3)
julia> subblocks(t)
subblocks(::TensorMap{Float64, Rep[U₁], 1, 1, Vector{Float64}}):
* (Irrep[U₁](0),) ← (Irrep[U₁](0),) => 1×1 StridedViews.StridedView{Float64, 2, Vector{Float64}, typeof(identity)}:
0.46446536222552703
* (Irrep[U₁](1),) ← (Irrep[U₁](1),) => 1×1 StridedViews.StridedView{Float64, 2, Vector{Float64}, typeof(identity)}:
0.8204502757997201
* (Irrep[U₁](2),) ← (Irrep[U₁](2),) => 1×1 StridedViews.StridedView{Float64, 2, Vector{Float64}, typeof(identity)}:
0.6469586476256173
* (Irrep[U₁](3),) ← (Irrep[U₁](3),) => 3×3 StridedViews.StridedView{Float64, 2, Vector{Float64}, typeof(identity)}:
0.526148 0.633339 0.404343
0.17951 0.704277 0.763412
0.249515 0.581565 0.757806
* (Irrep[U₁](4),) ← (Irrep[U₁](4),) => 1×1 StridedViews.StridedView{Float64, 2, Vector{Float64}, typeof(identity)}:
0.648902220665182
* (Irrep[U₁](5),) ← (Irrep[U₁](5),) => 3×3 StridedViews.StridedView{Float64, 2, Vector{Float64}, typeof(identity)}:
0.440466 0.977884 0.869157
0.741769 0.511166 0.448612
0.406724 0.245635 0.770315On top of that, I refactored some of the indexing functionality by splitting some of it as In particular, the idea is that the |
lkdvos
changed the title
subblocks iterator
|
@Jutho this should now also be ready for review. I think overall this is a lot more convenient, and we should probably include this before the docs rewrite as well |
| print(io, d, "-element ") | ||
| isdual(V) && print(io, "dual(") | ||
| print(io, type_repr(typeof(V))) | ||
| isdual(V) && print(io, ")") |
There was a problem hiding this comment.
I still think this d-element printing style is not really appropriate. When is this method called; is this in notebooks?
How about some printing style for an object like V = SU2Space(0=>3, 1=>2, 2=>1):
"Rep[SU₂] object with dimension 14 (reduced dimension 6)"
or for V'
"Rep[SU₂] dual object with dimension 14 (reduced dimension 6)"
where also the extra information in parentheses can be omitted if dim(V) == reduceddim(V).
Just a suggestion, let me know what you think.
There was a problem hiding this comment.
Doing another deep dive into the implementations and documentation, this is what I can find:
The different show methods are mostly used like this:
show(io, MIME"text/plain"(), x)is what gets called mostly in interactive usage, i.e. it ends up being the implementation when youdisplaysomething to an output that can support text. (I.e. REPL, or notebook output, ...). This is supposed to be for human consumption- Container types generally implement 3-argument show by calling
show(io, MIME"text/plain"(), x)for elementsx, with:compact => trueset. In this case you should avoid\nto not break the printing of the container. :limit => truecan be set to print…in place of most elements in a container.show(io, x)is the default implementation/fallback, which is typically used for computer consumption (i.e. parseable)
For example Array handles this for the 3-arg version by writing a summary first, and then, on new line(s) prints the data. If compact => true, the summary is omitted and the [x; y;; ...] single-line notation is used to show the data.
So, what I would like is:
- The 3-arg show for a space gives me a quick summary with the most important data, which is, in my opinion, the sectortype, some measure of size and the duality. Then, the sectors and dimensions can be used afterwards to fully specify the space
- The 3-arg show with
:compact => trueshould not contain any\n. We can decide to either showVect[I](x => d, ...)or show the summary, depending on what we think is the most useful information to show at a glance in a container of spaces. This is the thing that would get called e.g. when showing the spaces of an MPS, which you could think of as a container space
I don't actually feel to strongly about how this information is worded exactly, but I do think showing the dimension somewhere is a strict improvement, for example in MPS or PEPS methods, the relevant spaces to inspect are the virtual spaces, which tend to become completely unreadable when there are many sectors. In this case, simply truncating the sectors and going for the … reduces the screen clutter, but then there is no more information on the screen.
I have no real preference about the dim vs reduced dim, I argue that the information is mostly in comparing different values anyways (I want to see if my dimension increased or decreased during a DMRG run), but given that we are typically not using reduceddim in many places I feel like to not make this number confusing we would have to be a little explicit about it.
"Rep[SU₂] dual object with dimension 14 (reduced dimension 6)" seems a bit long, I think one of the dimensions is sufficient? I would also prefer to avoid the categoric "object" language when this isn't necessary, so maybe to suggest either alternative:
"(dual) Rep[SU₂] of dim d"
"(dual) Rep[SU₂] of reduceddim d"
This gives all information, would be Float dim-friendly since the floating point number is at the end, avoids (more) use of category language and is reasonably short. dim and reduceddim are idiomatic since we are already using that as a function name, so it cannot be confusing what this number is referring to.
There was a problem hiding this comment.
The use of 'object' was supposed to be such that only category-minded people (like you) think about categorical objects 😄 , and other people think of a computer-science object of type Rep[SU₂]. Though the possible occurance of dual then spoils that a little bit. I just think the one guideline should be not to take Array as an example, as really spaces are supposed to be like small bits types (even if GradedSpace isn't really. Furthermore, Array still prints out all elements even with :compact => true, so by that guideline we can still print all the sectors. But I agree that this is not what you want to see when an MPS is printed. However, I do think it is strange if ℂ^4 is printed just as this, and Rep[SU₂](...) is printed as dual Rep[SU₂] of dim d. I kind of dislike the implicit implication that it is an instance and not a type.
How about the following compromise: With :compact => true, a V::GradedSpace is printed as
type_repr(typeof(V)) * "(…)" * (isdual(V) ? "'" : "") * " with dim $(dim(V))"
Depending on the length of the information that you want to show, you can add additional information such as " with $(length(sectors(V)) sectors and dim $(dim(V))" but this is really up to you.
There was a problem hiding this comment.
I am still confused by the show methods. If you just have an interactive REPL session, it goes via display(V) and ends up calling show(stdout, MIME"text/plain", V). Is that than also the version that should check for :limit? So we might have :compact=>false but :limit=>true, so we would have a second line printing the sectors? In fact, for show(stdout, MIME"text/plain", V), I wouldn't mind if the sectors are printed underneath each other as in your original design.
The version without MIME type, show(io, V), is only called if you call explicitly show(V). That one, even for Array, prints out all information and is supposed to be parseable. So can that just remain the current version that prints out all the sectors? For Array, the only difference between :compact true or false in that case is that it truncates floating point numbers to less accuracy, but that doesn't really make sense for sector dimensions.
There was a problem hiding this comment.
Oh wait, show(io, V) does also listen to :limit. Pff, this is annoying.
There was a problem hiding this comment.
I think (this is my interpretation, it's all very badly defined/documented) that :compact means no linebreaks and optionally more dense representations (less floating point digits, or x=>y instead of x => y), and :limit means respecting the displaysize.
The version without MIME type is only called if you explicitly show(V) or print(io, V), where I think it is supposed to be parseable but can still be :limitted if you like
| @inline Base.view(t::AbstractTensorMap, sectors::Tuple{I, Vararg{I}}) where {I <: Sector} = | ||
| subblock(t, sectors) | ||
| @inline Base.view(t::AbstractTensorMap, f₁::FusionTree, f₂::FusionTree) = | ||
| subblock(t, (f₁, f₂)) |
There was a problem hiding this comment.
Do you think it is useful to have view methods? I kind of consider this to be AbstractArray specific. I am a bit hesitant to provide to many alternative interfaces.
There was a problem hiding this comment.
I think if I would redesign everything right now, I would have gone with view instead of getindex to begin with, since we are technically performing "method-piracy" on Base.getindex, which is supposed to yield a copy.
I don't want to change this now (that would be really painful), but I do find it easier to say "we have made the decision to make Base.getindex(::AbstractTensorMap) default to behave like Base.view(::AbstractTensorMap)", since these two concepts (as words) already exist and have meaning to me.
This is more or less the only reason why I added this, I don't really see many people using it, but I also don't think it hurts to have this here.
There was a problem hiding this comment.
First an aside, your remark about the "method piracy" on Base.getindex is probably valid for StridedView, though in that case I do think it is well documented that slicing it will produce a view instead of a new copy.
For AbstractTensorMap, I do not agree, because I do not consider Base.getindex as a method that is specific to AbstractArray, and therefore I do not think it should follow the same rules. Many types have a getindex that have nothing to do with AbstractArray. If you would have some kind of dictionary that stores blocks as values with fusion tree pairs as keys, it would be very natural that indexing with the key would return the original block, without making a copy. I think this is the analogy I have in my head when I think of (Abstract)TensorMap, so I really don't think of them as AbstractArrays. It seems ITensors have the same behavior: T[Block((1,3))] returns an AbstractArray subtype that is actually providing a view into the data.
In fact, I believe returning a view is necessary if we want some syntax like T[(f1, f2)] .= 1 to work. Here, it becomes messy with Array, since A[1:2, 1:3] .= value will also overwrite the data, but I really have no idea what this is lowered to. I assume something like setindex!.(Ref(A), value, ...) where I don't know what ... needs to be so that it is not considered as just a one-dimensional broadcast. But this anyways cannot work for our case.
So while I don't think our TensorMap getindex is problematic, I agree that using a separate method like subblock makes the distinction even more clear.
However, also recycling view for this is again hitting the fundamental disagreement we seem to have. I don't like to think of (Abstract)TensorMap as an AbstractArray subtype, and so I had opted not to use any methods that seem to be specific to AbstractArray in Julia base (ndims, size, axes, view, ...) for AbstractTensorMap. However, AbstractTensorMap definitely is a container type, and so the basic methods for general containers (getindex and setindex!) are available to us, with the key types and behavior that seems appropriate for our use case.
It seems ITensors are a bit in a mixed category: they listen to size and ndims, and you can index them with integer indices, but not with ranges and you cannot apply view. Do you have any insight into the user experience and typical user requests in that regard?
There was a problem hiding this comment.
I think most of the complaints surrounding ITensors seem to land in the same category: the lack of support for slicing is the most-often quoted nuisance, and the opaque manner in which the data is stored when introducing symmetries is not great.
Overall, my feeling is that people tend to expect these things to "just work", and they care less about the performance than they do about the convenience of not having to alter their habits of how to code or interact with arrays. I would say that people are receptive to the "this works, but if you want a fast/better implementation you have to work a little for it"
| #---------------------- | ||
| Base.ndims(t::AbstractTensorMap) = numind(t) | ||
| Base.size(t::AbstractTensorMap) = ntuple(Base.Fix1(size, t), numind(t)) | ||
| Base.size(t::AbstractTensorMap, i) = dim(space(t, i)) |
There was a problem hiding this comment.
I am still not convinced about the usefulness of this. This is mostly aimed at making it more friendly for newcomers? But shouldn't we stimulate them to just learn from the start that TensorMaps are not just AbstractArrays and that they should just check the documentation to learn about the interface. I am happy to for example have a summary of the interface in the README or whathever can help.
One reason why I think this might not be a good idea is that this forces length(t) = prod(size(t)) = dim(codomain) * dim(domain). But there might be an argument to be made for length(t) = dim(codomain ← domain) = dim(t). So once we establish this is part of our public interface, it will be very hard to revert this if we ever come up with a better use case for these definitions.
There was a problem hiding this comment.
I definitely agree with the sentiment to steer people towards our interface, but I don't think we currently have one for this exact concept, and (dims(codomain(t))..., dims(domain(t))...) is a bit less convenient in my opinion. Since I think it is quite obvious what size would return, it is a bit weird to me to get a MethodError. I don't think we as a library should be deciding if it is allowed to use size or not, and because of type piracy reasons this is basically the only place this method can be defined, so we might as well add it?
I have to say that I am not sure I agree that your reason is really related to this. I don't think length(t) = prod(size(t)) is expected, and I don't see why we should hold off on defining size just because there might be other functions that could have multiple meanings.
Ultimately, it's mostly just about being pragmatic. It's useful functionality, it's a short, easily available function name that is easy to remember and it's clear what it does, and I see very little harm in overloading it
There was a problem hiding this comment.
Would using dims(::HomSpace) be an adequate solution, or you really think users will want to use size? I have to say that this has never come up as an open issue on Github, but maybe users complain to you directly?
There was a problem hiding this comment.
I have had a couple people mention that they found it surprising that it doesn't work (technically, I quote: "this is why no one is using TensorKit")
I'm okay with dims(::HomSpace), (can we also have dims(::AbstractTensorMap)?), but still have a hard time motivating "we didn't implement size because or tensor type would look to much like an array which we want to avoid as it might cause confusion". Maybe I'm just reading too much into it, but this seems like a very easy thing to fix and just provide?
There was a problem hiding this comment.
Sure, if dims(::HomSpace) work, than dims(::AbstractTensorMap) must return the same. But I guess I will have to give up my fight against size then. Maybe we need an allowsize like CUDA.allowscalar ? 😄 .
There was a problem hiding this comment.
I especially look forward to the warning on the first call:
julia >size(::TensorMap)
WARNING: a TensorMap is not an AbstractArray and has a different interface; expect non-integer dimensions and other unexpected behavior. RTFM!There was a problem hiding this comment.
Jokes aside, I would actually be quite happy with a TensorKit.allowscalar to allow scalar getindex/setindex for abelian and getindex for non-abelian tensors too 😁
2 participants
These are various (WIP) opinionated show improvements, which I think improve overall quality of life. See also #301.
In the examples below, I tried to stick to
show(io, obj)being parse-able, whileshow(io, MIME"text/plain"(), obj)being read-able. The latter is what gets shown when you show something directly to the terminal, the former is the fallback, which for example shows up when you print arrays of these objects.Examples:
Graded spaces
I've limited the output of the sectors, and made the printing closer to how
Base.Vectorwould print:Before
After
Tensors
I've reworked the logic to condense some of the information, and moved printing the actual data to
blocks(t):Before
After
I would argue that already this is showing both more information and using a whole lot less space, but obviously this is subjective. Additionally, I quite like how clean the implementation is overall.
One thing I might still add is
subblocks(t), as a counterpart ofblocks(t), which would be equivalent to((f1, f2) => t[f1, f2] for (f1, f2) in fusiontrees(t)), such that I can also overload the show there and have the old behavior as an option.I also still need to tackle the adjoint and diagonal tensors :)