-
Notifications
You must be signed in to change notification settings - Fork 152
Add FieldMatrix: a rank-two tensor generalisation of FieldVector #626
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
Conversation
Should we generalize things more and have a |
@tkoolen that would indeed support all use cases. People from solid mechanics, for example, sometimes use rank-four stiffness tensors. However, performance for large static arrays is no good, right? |
Completely off-topic, but have you seen https://github.com/KristofferC/Tensors.jl ? |
This looks pretty neat. Agreed that it would be preferable to generalize to arbitrary dimensions if you're willing to take a go at that. In that case I imagine Somewhat tangential, but have you seen https://github.com/JuliaDiffEq/LabelledArrays.jl ? It's an interesting alternative approach where the array type is identified by the names of the fields. (LabelledArrays wasn't possible when we originally created |
@fredrikekre I have now :-) I had a quick glance and it seems that https://github.com/KristofferC/Tensors.jl supports at least all of my needs. Please correct me if I'm wrong, but I think types which inherit from
These limitations do not bother me because I don't need more, but I don't think this applies to everyone. Also I noticed strange behaviour when adding a literal value to a
I do not like that the result is heap allocated. |
@c42f Thanks and I also agree that it would be good to generalise to https://github.com/JuliaDiffEq/LabelledArrays.jl looks very promising indeed. I would be very interested in some benchmarks with functions from |
As of commit 89f40d8 The constructors might need some additional work and some actual testing of higher-rank tensors should be added. |
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.
Thanks for the update, that's a great generalization for basically the same amount of code.
One issue we need to sort out is column vs row major ordering in constructors and field naming in the examples. We can demonstrate some current inconsistencies with the Stress
example:
julia> struct Stress <: FieldMatrix{3, 3, Float64}
xx::Float64
xy::Float64
xz::Float64
yx::Float64
yy::Float64
yz::Float64
zx::Float64
zy::Float64
zz::Float64
end
# Unfortunately, constructing `Stress` strongly suggests row major ordering of elements
# but that's not what we get:
julia> Stress(1,2,3,
4,5,6,
7,8,9)
3×3 Stress:
1.0 4.0 7.0
2.0 5.0 8.0
3.0 6.0 9.0
# Indexing at row 1, col 2 returns a surprising result compared to the field ordering in the constructor:
julia> Stress(1,2,3,
4,5,6,
7,8,9)[1,2]
4.0
# This is also inconsistent with accessing the second field using `.xy`:
julia> Stress(1,2,3,
4,5,6,
7,8,9).xy
2.0
StaticArrays
assumes that arrays are stored in column major order (as does the getindex
implementation via getfield
). This means you need to flip the order of the fields in your example tensors if you want consistency between getindex(a, i,j)
and the field names.
Here's one possible way forward:
- Document that
FieldArray
should have column major field ordering unless people supply their owngetindex
andsetindex
. - Disable the default constructor for
FieldArray
s so that people can't make the row major formatting mistake I've got above. - Finally address Inconsistencies with multi-argument constructors #518 by inventing some syntax to allow writing general
StaticArray
literals. A constructor-like macro will probably be required.
Alternatively, we could
- Document that
FieldArray
should have column major field ordering, unless people supply their owngetindex
andsetindex
. - Create a constructor which transposes the input elements so that row major input ordering turns into column major data. But honestly this feels like it could be problematic and cause more issues in the long run than it solves. Again, see Inconsistencies with multi-argument constructors #518 for some of the difficulties we already have with constructors.
Some relevant thoughts about our constructor woes over at #518 (comment). With that proposal, the way to efficiently and unambiguously construct Stress(@SA[1 2 3;
4 5 6;
7 8 9]) |
Thanks for pointing this out @c42f, the order of the fields in the example in OP was an oversight of mine. It should have been:
The same modification should also be applied to I would not go the column-major way because row-major is the standard / more common in Julia. We could explicitly mention it in the documentation, however, I think column-major is expected behaviour. I fail to see the need for the macro in constructing an object of type
EDIT I meant to say column-major instead of row-major and vice versa. |
…with getindex and setindex
Excellent, thanks for sorting out the field ordering.
Column major is expected by the subset of Julia users who know to think about storage orders. However, the average user of multidimensional arrays would generally not need to know about this so it's worth mentioning in the doc strings.
This makes the compiler generate rather slow code compared to julia> using StaticArrays, BenchmarkTools
[ Info: Recompiling stale cache file /home/tcfoster/.julia/compiled/v1.1/StaticArrays/yY9vm.ji for StaticArrays [90137ffa-7385-5640-81b9-e52037218182]
julia> struct Stress <: FieldMatrix{3, 3, Float64}
xx::Float64
yx::Float64
zx::Float64
xy::Float64
yy::Float64
zy::Float64
xz::Float64
yz::Float64
zz::Float64
end
julia> @btime Stress(rand(3,3))
46.995 ns (1 allocation: 160 bytes)
3×3 Stress:
0.592865 0.26077 0.870697
0.900356 0.129086 0.6465
0.157575 0.422919 0.219663
julia> @btime Stress(@SArray(rand(3,3)))
17.234 ns (0 allocations: 0 bytes)
3×3 Stress:
0.20502 0.166018 0.699433
0.790852 0.96874 0.207043
0.292533 0.03907 0.831171 For cases where there's less work being done compared to generating random numbers this will only be more pronounced. |
Yes, I meant column-major instead of row-major and vice versa. I edited my post. Ah I didn't know it was slow. Then I agree with the macro as well. |
By the way, I hope you can you see what I mean about the perils of the multi-argument constructor? With the latest version: julia> using StaticArrays
julia> struct Stress <: FieldMatrix{3, 3, Float64}
xx::Float64
yx::Float64
zx::Float64
xy::Float64
yy::Float64
zy::Float64
xz::Float64
yz::Float64
zz::Float64
end
julia> Stress(1,2,3, # Column major ordering with misleading formatting
4,5,6,
7,8,9)
3×3 Stress: # Oops, interpreted as the transpose of the above
1.0 4.0 7.0
2.0 5.0 8.0
3.0 6.0 9.0 This isn't your fault! The problem of constructors is a difficult problem we've been struggling with for some time (see #518). However I think it's worth mentioning this potential pitfall somewhere. It would be possible to disallow the multi-argument constructor by default in an effort to prevent these issues though that might be heavy handed. It would be less bad with a shorter |
Yes, I do see the issue with the multi-argument constructor. I will make a mention of this in the documentation. I'm in for However, |
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.
I've left some suggestions for the documentation. Other than that, I think this is basically good to go.
Thanks for putting in the effort to make this a really solid contribution :-)
I'm with @c42f, thanks very much for the effort! |
Well anything is possible with an inner constructor but that doesn't help us here because any general constructors we define for |
FieldVector
already facilitates the creation of vector types which can be used insideVector{<:FieldVector}
arrays to represent vector fields. I also have the need to represent rank-two tensor fields (gradients of vector fields for example). Therefore, I have implemented theFieldMatrix
type to facilitate this. Others might find this useful too.Inheriting from this type makes it easy to create your own rank-two N x M tensor types in a similar fashion that
FieldVector
does for vectors types. Here is an example:Tests can be found in
test/FieldMatrix.jl
. These tests mimick the tests fromtest/FieldVector.jl
. All tests fromtest/runtests.jl
(withtest/FieldMatrix.jl
included) passed without any warning/error.Please let me know whether this is useful and/or improvements are needed before this can be accepted. For example, I'm not too sure about the constructors I provided. They work, but might not yet be optimal performance-wise.