Skip to content

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.

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?

/// looks more like Python 'str' and doesn't require a boolean argument 'true' in order
/// to discriminate.
/// </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.

2 participants