Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Tensorboard Issue #932 #936

Merged
merged 27 commits into from Apr 25, 2023
Merged

Tensorboard Issue #932 #936

merged 27 commits into from Apr 25, 2023

Conversation

ChengYen-Tang
Copy link
Contributor

@ChengYen-Tang ChengYen-Tang commented Feb 24, 2023 •
edited

#932

  • add_text
  • add_histogram
  • add_video
  • add_image

@ChengYen-Tang
Copy link
Contributor Author

@dotnet-policy-service agree

@ChengYen-Tang
Copy link
Contributor Author

@NiklasGustafsson
I open a PR and want you to help me code review.
I finished the add_text function, but I don't know how to unit test it.

Trying to implement other functions.

@NiklasGustafsson
Copy link
Contributor

NiklasGustafsson commented Feb 24, 2023 •
edited

As far as unit tests goes, you can't do much besides testing that it doesn't blow up on things like a null string, an empty string, non-ANSI strings, etc. You have to manually verify that it works and that Tensorboard can read it, for a few instances of those cases that make sense -- a null or empty string doesn't, for example.

NiklasGustafsson
NiklasGustafsson requested changes Feb 28, 2023
View reviewed changes
src/TorchSharp/Utils/tensorboard/SummaryWriter.cs Outdated Show resolved Hide resolved
src/TorchSharp/Utils/tensorboard/Utils.cs Outdated Show resolved Hide resolved
src/TorchSharp/Utils/tensorboard/Utils.cs Outdated Show resolved Hide resolved
src/TorchSharp/Utils/tensorboard/Summary.cs Outdated Show resolved Hide resolved
src/TorchSharp/Utils/tensorboard/Summary.cs Outdated Show resolved Hide resolved
@ChengYen-Tang
Copy link
Contributor Author

Note: I have not tested these codes.
I'll start testing when I'm done with add_video

NiklasGustafsson
NiklasGustafsson requested changes Mar 1, 2023
View reviewed changes
src/TorchSharp/Utils/tensorboard/Summary.cs Show resolved Hide resolved
src/TorchSharp/Utils/tensorboard/Summary.cs Outdated Show resolved Hide resolved
src/TorchSharp/Utils/tensorboard/Summary.cs Outdated Show resolved Hide resolved
ChengYen-Tang
ChengYen-Tang commented Mar 3, 2023
View reviewed changes
Copy link
Contributor Author

@ChengYen-Tang ChengYen-Tang left a comment

Choose a reason for hiding this comment

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

looks good

src/Examples/TensorboardExample/Tensorboard.cs Outdated Show resolved Hide resolved
src/Examples/TensorboardExample/Tensorboard.cs Outdated Show resolved Hide resolved
@NiklasGustafsson
Copy link
Contributor

NiklasGustafsson commented Mar 3, 2023 via email

@NiklasGustafsson
Copy link
Contributor

NiklasGustafsson commented Mar 3, 2023 •
edited

Earlier, you talked about drawing boxes in images -- where are you doing that? That should be fairly straightforward just using Tensors, right? I would think the Tensor type is great for image manipulation, and it's HW-accelerated, too.

What I mean is:

var t = zeros(1, 3, 32, 32, uint8);

var colon = TensorIndex.Colon;
var hLength = TensorIndex.Slice(2, 30);
var vLength = TensorIndex.Slice(2, 30);

t[colon, colon, 2, hLength] = (byte)0xFF;
t[colon, colon, 29, hLength] = (byte)0xFF;
t[colon, colon, vLength, 2] = (byte)0xFF;
t[colon, colon, vLength, 29] = (byte)0xFF;

Should, unless I'm mistaken, draw a white box on an otherwise black background, two pixels from the edge. With .NET Core, it looks nicer since we can use language support for System.Range, but TorchSharp has to support both .NET Core and .NET FX.

@ChengYen-Tang
Copy link
Contributor Author

Earlier, you talked about drawing boxes in images -- where are you doing that? That should be fairly straightforward just using Tensors, right? I would think the Tensor type is great for image manipulation, and it's HW-accelerated, too.

What I mean is:

var t = zeros(1, 3, 32, 32, uint8);

var colon = TensorIndex.Colon;
var hLength = TensorIndex.Slice(2, 30);
var vLength = TensorIndex.Slice(2, 30);

t[colon, colon, 2, hLength] = (byte)0xFF;
t[colon, colon, 29, hLength] = (byte)0xFF;
t[colon, colon, vLength, 2] = (byte)0xFF;
t[colon, colon, vLength, 29] = (byte)0xFF;

Should, unless I'm mistaken, draw a white box on an otherwise black background, two pixels from the edge. With .NET Core, it looks nicer since we can use language support for System.Range, but TorchSharp has to support both .NET Core and .NET FX.

Because I found that pytorch SummaryWriter does not use the function to require a bounding box...so, I did not implement this method

https://pytorch.org/docs/stable/tensorboard.html#torch.utils.tensorboard.writer.SummaryWriter
https://github.com/pytorch/pytorch/blob/master/torch/utils/tensorboard/summary.py#L450-L478

ChengYen-Tang
ChengYen-Tang commented Mar 5, 2023
View reviewed changes
Copy link
Contributor Author

@ChengYen-Tang ChengYen-Tang left a comment

Choose a reason for hiding this comment

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

The program crashes in a certain state, but I can't find the reason...

src/Examples/TensorboardExample/Tensorboard.cs Outdated Show resolved Hide resolved
src/Examples/TensorboardExample/Tensorboard.cs Outdated Show resolved Hide resolved
@GeorgeS2019
Copy link

@ChengYen-Tang

FYI: dotnet/interactive#2825

@ChengYen-Tang ChengYen-Tang mentioned this pull request Mar 16, 2023
Merged
@GeorgeS2019
Copy link

@ChengYen-Tang

  • add_histogram <== done? what else missing?

@ChengYen-Tang
Copy link
Contributor Author

@ChengYen-Tang

  • add_histogram <== done? what else missing?

only this problem
#936 (comment)

ChengYen-Tang

This comment was marked as outdated.

Sign in to view

@NiklasGustafsson
Copy link
Contributor

@NiklasGustafsson I found that these codes seem to come from the source code of Android. Are you know well with Android source code licence?

Different parts of Android are covered by different licenses, so it's hard to say.

ChengYen-Tang
ChengYen-Tang commented Apr 24, 2023
View reviewed changes
Copy link
Contributor Author

@ChengYen-Tang ChengYen-Tang left a comment •
edited

Choose a reason for hiding this comment

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

@NiklasGustafsson ,
I found that these codes seem to come from the source code of Android.
Are you know well with Android source code licence?

https://cs.android.com/android/platform/superproject/+/master:external/glide/LICENSE

src/TorchSharp/Utils/tensorboard/GifEncoder/Encoder.cs Show resolved Hide resolved
src/TorchSharp/Utils/tensorboard/GifEncoder/LZWEncoder.cs Show resolved Hide resolved
src/TorchSharp/Utils/tensorboard/GifEncoder/NeuQuant.cs Show resolved Hide resolved
@ChengYen-Tang
Copy link
Contributor Author

The Unisys LZW patent seems to have expired and we can use it without restriction?

@NiklasGustafsson
Copy link
Contributor

The Unisys LZW patent seems to have expired and we can use it without restriction?

I'm consulting with some internal people to see how we proceed.

@NiklasGustafsson
Copy link
Contributor

The Unisys LZW patent seems to have expired and we can use it without restriction?

I'm consulting with some internal people to see how we proceed.

Okay, so I heard back -- if we add a section (or two) to the THIRD-PARTY-NOTICES.txt file that calls out the source of this code, we should be fine.

NiklasGustafsson
NiklasGustafsson requested changes Apr 25, 2023
View reviewed changes
src/Examples/Program.cs Outdated
namespace TorchSharp.Examples
{
public static class Program
{
public static void Main(string[] args)
public static async Task Main(string[] args)
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems unnecessary to make this asynchronous. The program is a console app, and you have to await the result, anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

src/TorchSharp/Tensor/torch.OtherOperations.cs
@@ -628,17 +628,6 @@ public static Tensor[] meshgrid(IEnumerable<Tensor> tensors, string indexing = "
/// </summary>
public static Tensor roll(Tensor input, ReadOnlySpan<long> shifts, ReadOnlySpan<long> dims = default) => input.roll(shifts, dims);

// https://pytorch.org/docs/stable/generated/torch.searchsorted
Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because in case of 1 this code is ambiguous with 2.

src/TorchSharp/Utils/tensorboard/Enums/HistogramBinSelector.cs
@@ -0,0 +1,13 @@
namespace TorchSharp.Utils.tensorboard.Enums
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this enum declared in two places?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This enum has one more tensorflow item.

src/TorchSharp/Utils/tensorboard/GifEncoder/LZWEncoder.cs Outdated
{
public static partial class tensorboard
{
public static partial class GifEncoder
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this class should be public.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

src/TorchSharp/Utils/tensorboard/GifEncoder/NeuQuant.cs Outdated
{
public static partial class tensorboard
{
public static partial class GifEncoder
Copy link
Contributor

Choose a reason for hiding this comment

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

See previous comment. Should this be public?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@NiklasGustafsson
Copy link
Contributor

Note: I have not tested these codes. I'll start testing when I'm done with add_video

@ChengYen-Tang, I presume it's been tested now.

@ChengYen-Tang
Copy link
Contributor Author

Note: I have not tested these codes. I'll start testing when I'm done with add_video

@ChengYen-Tang, I presume it's been tested now.

Yes, tested

@NiklasGustafsson NiklasGustafsson linked an issue Apr 25, 2023 that may be closed by this pull request
SummaryWriter is not fully functional #932
Closed
@NiklasGustafsson
Copy link
Contributor

I'll merge as soon as I get a successful build in Azure Pipelines (intermittent failures that have nothing to do with the library).

@NiklasGustafsson NiklasGustafsson merged commit b87e315 into dotnet:main Apr 25, 2023
2 checks passed
@GeorgeS2019 GeorgeS2019 mentioned this pull request Apr 27, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NiklasGustafsson NiklasGustafsson NiklasGustafsson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SummaryWriter is not fully functional
3 participants
@ChengYen-Tang @NiklasGustafsson @GeorgeS2019