small patch to complex, real and imag #173
Conversation
src/tensors/tensor.jl
Outdated
| return t | ||
| else | ||
| return copy!(similar(t, complex(scalartype(t))), t) | ||
| return TensorMap(complex(t.data), space(t)) |
There was a problem hiding this comment.
I'm not a big fan of having TensorMap hard-coded here. I think similar, by definition, should return a mutable tensor to copy the data into, which by default returns a TensorMap, similar to how Array is the default for similar(::AbstractArray), but I need to overload this for BlockTensorMap, which is easier if everything passes through similar.
There was a problem hiding this comment.
It feels like it's the second time I make this mistake. I forget that this method works on AbstractTensorMap, whereas it is in tensormap.jl. Maybe I should move it. However, I do think having the simple TensorMap specialisation around is also useful.
src/tensors/tensor.jl
Outdated
| # TODO: should we reformulate the old checks in terms of `sectorscalartype` | ||
| # For anyonic categories, complex numbers typically come into play only in the | ||
| # braiding and not in the fusion. Hence, it can make sense to work with real | ||
| # tensors if no braiding is required. |
There was a problem hiding this comment.
I wouldn't mind having real/imag work even if the fusion data is not real. I understand that it might lead to bugs here and there, but for example for singular values, it is quite natural to allow real numbers (since there are no fusion tensors anyways).
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #173 +/- ##
==========================================
- Coverage 81.21% 80.93% -0.29%
==========================================
Files 41 41
Lines 5005 5015 +10
==========================================
- Hits 4065 4059 -6
- Misses 940 956 +16 ☔ View full report in Codecov by Sentry. |
| # that the real/imaginary part of a tensor `t` can be obtained by just taking | ||
| # real/imaginary part of the degeneracy data. | ||
| if isreal(sectortype(t)) | ||
| return TensorMap(real(t.data), codomain(t), domain(t)) |
There was a problem hiding this comment.
So this one slipped through in the previous change, as it should actually also only work for TensorMap arguments. That one is not entirely straightforward to define for AbstractTensorMap.
|
AD CI on mac is really going nuts; not sure if this now changed more due to this PR, or whether it is a random glitch. I run the test locally, and if successful, I think this is ok to merge? |
No description provided.