Skip to content

Comments

TensorTypeExtensions: Added conversion between Tensor to primitive C# types instead of throwing NotSupportedException#4290

Closed
Nucs wants to merge 7 commits intodotnet:masterfrom
Nucs:tensor-type-extension-widen-support
Closed

TensorTypeExtensions: Added conversion between Tensor to primitive C# types instead of throwing NotSupportedException#4290
Nucs wants to merge 7 commits intodotnet:masterfrom
Nucs:tensor-type-extension-widen-support

Conversation

@Nucs
Copy link

@Nucs Nucs commented Oct 3, 2019

  • Added conversion between Tensor to native C# types instead of throwing NotSupportedException
  • Fixed proper reading of TF_STRING
  • Added overloads that support TF_STRING (string does not meet T generic constraint unmanaged

Nucs added 2 commits October 3, 2019 23:08
…g NotSupportedException

- Fixed proper reading of TF_STRING
- Added overloads that support TF_STRING
@Nucs Nucs requested a review from a team as a code owner October 3, 2019 20:14
@dnfclas
Copy link

dnfclas commented Oct 3, 2019

CLA assistant check
All CLA requirements met.

@Nucs Nucs changed the title TensorTypeExtensions: Added conversion between Tensor to native C# types instead of throwing NotSupportedException TensorTypeExtensions: Added conversion between Tensor to primitive C# types instead of throwing NotSupportedException Oct 3, 2019
internal static class TensorTypeExtensions
{
public static void ToScalar<T>(this Tensor tensor, ref T dst) where T : unmanaged
{
Copy link
Member

@codemzs codemzs Oct 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this handle string tensors? #Closed

Copy link
Author

@Nucs Nucs Oct 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was sure I had String-Tensor -> T conversion implemented, Added it. #Closed

}
}

public static void CopyTo<T>(this Tensor tensor, Span<T> destination) where T : unmanaged
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public static void CopyTo(this Tensor tensor, Span destination) where T : unmanaged [](start = 8, length = 89)

There is a lot of unsafe code in here that does memory copy/manipulation, have you considered using Span.CopyTo(Span) and Span.Slice(int,int) to achieve the same?

Copy link
Author

@Nucs Nucs Oct 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a point, I perform all the checks necessary beforehand. The code is safe. Any use of Span will result in unnecessary overhead.
Plus, is there a way to perform cast from one dtype to an other using only Span?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@codemzs I can replace:
var sdst = (bool*) Unsafe.AsPointer(ref destination.GetPinnableReference());
With:
fixed (byte* sdst = MemoryMarshal.Cast<T, byte>(destination))

They do a similar logic just with more overhead.

Nucs added 3 commits October 4, 2019 00:10
- CopyTo<T>: Added `fixed` on destination to prevent GC from moving.
- Fixed wrong argument passing to Converts.ChangeType<T>
- Reformat
@codemzs
Copy link
Member

codemzs commented Oct 7, 2019

@Nucs Thanks for the change but I'm curious why are we making this change? Where exactly will it be used in the code base? This is an internal class btw.

@Nucs
Copy link
Author

Nucs commented Oct 7, 2019

@codemzs, I assumed at this comment that the upvote was an answer to my question there. Should've asked for a more explicit answer 😅.
Assuming conversion between Tensor of type X to T of type Y is not needed then this PR can be closed. Currently this case throw NotSupportedException.
ToScalar has a unmanaged constraint but If you need conversion from scalar Tensor to System.String then the implementation is here.

Eitherway I'll probably add this code to tensorflow.Tensor.

@Nucs Nucs closed this Oct 7, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Mar 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants