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.

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

Add numpy-style ToString method #572

Merged
merged 13 commits into from
Apr 19, 2022
Merged

Add numpy-style ToString method #572

merged 13 commits into from
Apr 19, 2022

Conversation

dayo05
Copy link
Contributor

@dayo05 dayo05 commented Mar 30, 2022

New method is exists on Tensor::str(). I'll create more tests for that.

@@ -5369,6 +5369,79 @@ public override string ToString()
return sb.ToString();
}

/// <summary>
Copy link
Contributor

Choose a reason for hiding this comment

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

This belongs in TensorExtensionMethods, I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another possibility is to add a 'style' argument to the verbose ToString() that currently takes a boolean.

Instead of

public string ToString(bool withData, string fltFormat = "g5", int width = 100, CultureInfo? cultureInfo = null)

it would be something like this:

public enum TensorStringStyle { Metadata, Julia, Numpy }

public string ToString(TensorStringStyle style, string fltFormat = "g5", int width = 100, CultureInfo? cultureInfo = null) 
{
    if (style == TensorStringStyle.Metadata) return ToString();
    ...
}

// For backward compat:

public string ToString(bool withData, string fltFormat = "g5", int width = 100, CultureInfo? cultureInfo = null) 
{
    return ToString(withData ? TensorStringStyle.Julia : TensorStringStyle.Metadata, fltFormat, width, cultureInfo);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then how about this?

public static TensorStringStyle DefaultOutputStyle = TensorStringStyle.Metadata;

            public override string ToString() => ToString(DefaultOutputStyle);

            public string ToString(TensorStringStyle style, string fltFormat = "g5", int width = 100,
                CultureInfo? cultureInfo = null) => style switch {
                    TensorStringStyle.Metadata => ToMetadataString(),
                    TensorStringStyle.Julia => ToJuliaString(fltFormat, width, cultureInfo),
                    TensorStringStyle.Numpy => ToNumpyString(this, ndim),
                    _ => throw new InvalidEnumArgumentException("Not supported type")
                };

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure. Don't you want CultureInfo and fltFormat on the NumPy string?

src/TorchSharp/Tensor/Tensor.cs Outdated Show resolved Hide resolved
@@ -41,7 +48,7 @@ public static Modules.Parameter AsParameter(this Tensor tensor)
/// </remarks>
public static string str(this Tensor tensor, string fltFormat = "g5", int width = 100)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this should also take a style argument, but defaulted to 'Julia,' not 'Metadata' There could also be a 'npstr()' that defaults to NumPy. Same for print().

@NiklasGustafsson
Copy link
Contributor

NiklasGustafsson commented Apr 14, 2022

New test failures. This time, I think it's because of the tests doing this:

.Select(x => x * 2.5 + 5)

which leads to rounding differences in various runtimes.

For a test like this, I think you probably want to have a fixed collection of numbers, not involving any FP arithmetic, so that you avoid the risk of rounding or other arithmetic-related errors.

@dayo05 dayo05 marked this pull request as ready for review April 19, 2022 14:19
@NiklasGustafsson NiklasGustafsson merged commit a6feaaf into dotnet:main Apr 19, 2022
@NiklasGustafsson
Copy link
Contributor

@dayo05 -- I just merged your changes. Looking good!

With this, I will prepare and make a new release later today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants