WIP: Suggestion new tensor creation - #41
Conversation
marcelluethi
left a comment
There was a problem hiding this comment.
I like the new interface a lot. It makes things both cleaner and more terse.
One thing I was wondering tough:
val t1 = Tensor(Shape(Axis[A] -> 3)).fromArray(Array(1f, 2f, 3f))
val t2 = Tensor(Shape(Axis[A] -> 3)).fill(1.0)
val t3 = Tensor(Shape(Axis[A] -> 3)).fill(true)
// why
val t4 = Tensor.randn(Shape(Axis[A] -> 3))(Random.Key(42))
// and not
val t5 = Tensor(Shape(Axis[A] -> 3)).randn(Random.Key(42))
The last line could be easily implemented by just moving the randn function to the factory. Is there a reason why you did not choose to do that?
|
I will add this. randn is something I want to change (to support any distribution). However, this could also be part of future changes. So thats why I overlooked it. So I will update randn. |
|
Great - if failing tests are addressed and randn is supported, it seems ready to merge. Regarding supporting all distributions. I think it is good to postpone that, as this doesn't seem to be easy. Currently distributions need a shape to be constructed. Once the shape of a distribution is fixed, sampling is easy - so we wouldn't even need convenience functions. Maybe this is the reasons that most libraries just allow randn (standard normal) and rand (uniform 0, 1) |
|
We currently use randn nowhere (except for Tensor0 in RandomSuite). I would suggest removing "randn" for now and keeping the Normal.standardNormal(shape).sample(key) syntax. // Current
weight = Normal.standardNormal(Shape(inputDim)).sample(paramKey),
// randn
weight = Tensor(Shape(inputDim)).randn(paramKey), |
We discussed that the current Tensor creation API can be improved.
This PR suggests a new API similar to what we discussed.
Additionally, it provides more flexibility to create tensors with various dtypes.
New API
Change position of shape Tensor.
Replaced const, zeros, ones with fill:
Fix Execution Type to allow dtype definitions in fill
We might have to discuss the API for chaning dtype (see below), as
Tensor(shape).fromArray(pixels, dtype=DType.UInt8)would seem more natural. However, I suggest moving this discussion to a later stage and focusing on the "default-dtype" API for now. The motivation for a type class is to change the behavior globally, e.g., switching from float32 to float64 in a simulation...
However, this PR did not change the ExecutionType API, just cleaned up its implementation so it works with the new fill operation (zeros, ones, const did not support changing dtypes). One minor change was changing Array[Byte] to be mapped to int8 by default not uint8, to be in line what the Scala type means.
Some conceptual things:
Code changes to check
As most files have been touched I highlight the main changes here: