Fixing Broadcasting Loop #29
Conversation
|
|
||
| private TensorShape(nint flattenedLength, nint linearLength, scoped ReadOnlySpan<nint> lengths, scoped ReadOnlySpan<nint> strides, scoped ReadOnlySpan<int> linearRankOrder, int rank) | ||
| { | ||
| Debug.Assert((linearLength == 0) || ((flattenedLength % linearLength) == 0)); |
There was a problem hiding this comment.
This one should be correct. If we're calling this constructor its because we've already validated everything and so either flattenedLength == linearLength (no broadcast) or flattenedLength % linearLength == 0 (broadcast, so linearLength should be a perfect multiple of flattenedLength)
There was a problem hiding this comment.
In the case it is a broadcast, then it is very likely linearLength <= flattenedLength.
However, this might be missing the case of large strides, in which case linearLength >= flattenedLength. That would require us also taking in the flags and handling the scenario
There was a problem hiding this comment.
Its also called during TensorShape.Slice, in which case the linear length of the original memory may be longer than the new flattened length.
the check how it was before fails some valid slice checks. So maybe the new one isn't quite right, but the old one is off as well.
There was a problem hiding this comment.
As part of Slice, the linearLength should be updated to be the new minimumLinearLength so that this isn't an issue.
In general we don't expect linearLength to be the original backing storage length, as that is potentially dangerous and can allow "leaking" of details that weren't intended.
Instead, we expect it to track the amount of linear memory that is actually needed by the array. Thus an int[] of length 20 can be used to create a 2x2 matrix and linearLength would be at least 1, most likely 4, and up to 20 with flattenedLength always being 4.
| public nint AdjustToNextIndex(in TensorShape destinationShape, nint linearOffset, Span<nint> indexes) | ||
| { | ||
| Debug.Assert(indexes.Length >= Rank); | ||
| Debug.Assert(indexes.Length == destinationShape.Rank); |
There was a problem hiding this comment.
When would this not be correct? We expect that destinationShape be either this or it be the shape we're "compatible with" and therefore broadcasting to.
There was a problem hiding this comment.
Its probably valid now. at one point during my itteration of changes it wasn't so I removed it. Let me re-add it.
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/TensorShape.cs
Outdated
Show resolved
Hide resolved
| if (index < length) | ||
| { | ||
| break; | ||
| return linearOffset; |
There was a problem hiding this comment.
We've talked about this logic a few times. A small comment covering why returning linearOffset here vs resetting back to -strides[^1] as the final would be beneficial
There was a problem hiding this comment.
Added, let me know your thoughts.
There was a problem hiding this comment.
This isn't quite the comment I was thinking of, I think we need one giving a simple example of like src = length: [...]; stride: [...]; dst = length: [...]; stride: [...] (or similar) which explains why such iteration logic is then correct.
In general the first loop handles every index that exists in src while anything in the second loop handles additional indexes in dst. The indexes in dst will only flip over in the case that the most significant index in src rolls over, at which point every index from src will be 0 and we should be repeating the data from src.
So it's still not clear when returning linearOffset would be correct, since we should be at an index that only exists in the destination due to a broadcast.
| TensorShape result = new TensorShape( | ||
| flattenedLength, | ||
| linearLength, | ||
| LinearLength - computedOffset, |
There was a problem hiding this comment.
Is this correct in all scenarios?
In the main constructor loop I've got it computing a minimumLinearLength based on the stride * length (where-as flattenedLength is flattenedLength *= stride).
I think it would be good to normalize to a similar pattern between the two, to help avoid any bugs or edge cases.
There was a problem hiding this comment.
I think this will be correct? Before linearLength wasn't being set at all so it was ending up 0. Now it should take the length of the original minus the length of the offset. Though I've only seen the offset be 0 (which was correct for what i was testing) so that will need further verification.
michaelgsharp
force-pushed
the
tensor-finishing
branch
from
7120f8b to
62b61b6
Compare
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/Tensor_1.cs
Show resolved
Hide resolved
…rious correctness fixes, and stability improvements (dotnet#114927) * Refactor Tensor to be more reusable and validate appropriate state * Handle Equals, GreaterThan, GreaterThanOrEqual, LessThan, LessThanOrEqual, and the *All/*Any variants * Many implementations correctly swapped to new form. (#25) * Refactor Tensor to be more reusable and validate appropriate state * finishing tensor primitives work --------- Co-authored-by: Tanner Gooding <[email protected]> * more tensors updates (#26) * Resolve a few build failures * Ensure SetSlice and ToString are working as expected * Tensors lastfew (#27) * only couple left * pausing for food * fixed rented buffer * squeeze/unsqueeze * set slice/ split * only 2 left * Minor cleanup of the Tensor files * Ensure that tensor tests are building * Resolving various build failures due to API compatibility * Ensure flattendLength is adjusted after the stride is set for that dimension * Ensure that we set linearLength if -1 is passed in when strides is empty * Ensure that the first index is correct * Cleanup to ensure iteration and construction initializes correctly * Ensure that broadcasting is allowed to be in any stride position * Have AreCompatible handle empty shapes * Ensure IndexOutOfRangeException is thrown for invalid indexes * Ensure that the stride is set to 0 when the length of a dimension is 1, so embedded broadcasting works * Fixing Broadcasting Loop (#29) * Fixing Broadcasting Loop * fixes from pr coments * squeeze fixed * unsqueeze * set slice * more tensor fies * Ensure that minimumLinearLength is actually the minimum * Ensure the rented buffer is cleared * Fix the AreCompatible checks * Tensor finishing (#30) * stack working * more tensor tests working * fix factory create tests * only2 tests left * Update src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/TensorShape.cs * Update compatibility suppressions * transpose working * reverse working * Revert the unnecessary sln changes * Remove an unnecessary using --------- Co-authored-by: Michael Sharp <[email protected]> Co-authored-by: Michael Sharp <[email protected]>
2 participants
No description provided.