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

Fixing default print newline #1415

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ozanMSFT
Copy link

Fixes #1414

We're attempting to add back the default newLine as \n to have correct results in Jupyter notebooks.

@yueyinqiu

This comment was marked as resolved.

@yueyinqiu
Copy link
Contributor

yueyinqiu commented Nov 26, 2024

A hardcoded "\n" might not be that feasible since we may like "\r\n" or something else on other platforms.

And I suppose what we should do is to switch the default value from "" to null. Then it will use torch.newLine:

var nl = newLine is null ? torch.newLine : newLine;

And... These lines are useless since we are later passing nl instead of newLine:

if (String.IsNullOrEmpty(newLine))

So maybe remove it?

// Assert
var result = sw.ToString().Trim();
Assert.Equal(expectedOutput, result);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

By the way maybe we shall set the original console.out back? I don't know ((

@ozanMSFT
Copy link
Author

@dotnet-policy-service agree company="Microsoft"

@ozanMSFT
Copy link
Author

A hardcoded "\n" might not be that feasible since we may like "\r\n" or something else on other platforms.

And I suppose what we should do is to switch the default value from "" to null. Then it will use torch.newLine:

var nl = newLine is null ? torch.newLine : newLine;

And... These lines are useless since we are later passing nl instead of newLine:

if (String.IsNullOrEmpty(newLine))

So maybe remove it?

Thanks for the comments. I've used \n to be consistent with other methods within TensorExtensionMethods.cs

Maybe we can consider to replace others to null to force OS to use its default new line but I think it can be a scope for another PR with your other suggestions for refactoring.

For this one, only focusing on fixing print issue.

@yueyinqiu
Copy link
Contributor

yueyinqiu commented Nov 27, 2024

Oh I didn't notice that. Maybe we shall reconsider that later.

@ozanMSFT ozanMSFT force-pushed the users/ozanMSFT/fix-print-newline branch from fe3ba0d to 6d081d9 Compare November 27, 2024 12:19
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.

tensor.print() - Missing default "newLine" Parameter
2 participants