java icon indicating copy to clipboard operation
java copied to clipboard

Added Tensor.dataToString()

Open cowwoc opened this issue 4 years ago • 31 comments
trafficstars

Fixes https://github.com/tensorflow/java/issues/268

cowwoc avatar Apr 03 '21 05:04 cowwoc

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

google-cla[bot] avatar Apr 03 '21 05:04 google-cla[bot]

@googlebot I signed it!

cowwoc avatar Apr 03 '21 05:04 cowwoc

@rnett Please review.

Apologies in advance. The implementation got hairy once I added maxWidth support. Please refactor this as you see fit to improve readability.

cowwoc avatar Apr 03 '21 05:04 cowwoc

The build failure doesn't seem to be related to my changes. Please confirm.

cowwoc avatar Apr 03 '21 06:04 cowwoc

The quick build failing in javadoc generation is a known issue we're working on in other PRs.

Craigacp avatar Apr 03 '21 13:04 Craigacp

Looks good, I have a few documentation requests and a request for float formatting. Did you consider adding an Operand.dataToString() default method that calls asTensor().dataToString() (or similar)?

Yes, I could do that. Are you saying that you would leave the main implementation on Tensor and simply add a convenience method on Operand that redirects to it?

cowwoc avatar Apr 04 '21 18:04 cowwoc

FYI, I decided to remove trailing commas after closing brackets. I think it looks better this way and I think that tf.print does the same. In order words:

[
  [1, 2],
  [3, 4],
  [5, 6]
]

Will now show up as:

[
  [1, 2]
  [3, 4]
  [5, 6]
]

cowwoc avatar Apr 04 '21 19:04 cowwoc

Looks good, I have a few documentation requests and a request for float formatting. Did you consider adding an Operand.dataToString() default method that calls asTensor().dataToString() (or similar)?

Yes, I could do that. Are you saying that you would leave the main implementation on Tensor and simply add a convenience method on Operand that redirects to it?

Yeah, that's exactly what I meant.

rnett avatar Apr 04 '21 19:04 rnett

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

google-cla[bot] avatar Apr 06 '21 02:04 google-cla[bot]

Other than the small comments, I made a PR (cowwoc#1) against your branch with additional data type tests, and (because of said tests) wrapping string elements in quotes. Use the PR if you want, but regardless, both of those would be good to have.

Merged. Thank you.

Let me know if there is anything else. Otherwise, how do we fix the quick-build so I can merge this PR?

cowwoc avatar Apr 06 '21 02:04 cowwoc

The test failure will be fixed in #276. The Javadoc failures we're still working on. Once #276 is merged in then I'll rerun the action and check that the tests complete.

Craigacp avatar Apr 06 '21 17:04 Craigacp

@googlebot I consent.

rnett avatar Apr 06 '21 18:04 rnett

@JimClarke5 Any chance you could pick up the torch (apply the changes that Karl asked for) on my behalf? I won't have time to pick this up for another week or two.

cowwoc avatar Apr 13 '21 16:04 cowwoc

@cowwoc OK, one problem I have is I need to work with my latest version of the xxxSession classes that I am using for Models and Layers, I assume you could cherry pick the Tensor related changes.

JimClarke5 avatar Apr 13 '21 16:04 JimClarke5

@cowwoc OK, one problem I have is I need to work with my latest version of the xxxSession classes that I am using for Models and Layers, I assume you could cherry pick the Tensor related changes.

@JimClarke5 You should be able to fork my PR, merge in your changes and push a new PR. I will approve it which should merge the changes back into this initial PR.

cowwoc avatar Apr 13 '21 18:04 cowwoc

I understand that, but I need to test with my latest code first.

JimClarke5 avatar Apr 13 '21 18:04 JimClarke5

Will Operand.dataToString() work in Graph mode with out first doing a try catch?

try (Tensor tensor = session.runner().fetch(input).run().get(0)) {
     tensor.dataToString()
}

JimClarke5 avatar Apr 13 '21 19:04 JimClarke5

Would we want the printed values enclosed in braces ( {} ) rather than square brackets( []}. Braces make it easier to copy and past into Java code.

JimClarke5 avatar Apr 13 '21 19:04 JimClarke5

I think we should pattern the output after java.util.Arrays.deepToString(), which takes us back to brackets with commas.

float[][][] f = new float[][][] {
            {{1,2}, {3,4}},
            {{5, 6}, {7, 8}},
            {{9, 10}, {11, 12}}
};   
System.out.println(Arrays.deepToString(f));
// [[[1.0, 2.0], [3.0, 4.0]], [[5.0, 6.0], [7.0, 8.0]], [[9.0, 10.0], [11.0, 12.0]]]

JimClarke5 avatar Apr 13 '21 19:04 JimClarke5

Will Operand.dataToString() work in Graph mode with out first doing a try catch?

try (Tensor tensor = session.runner().fetch(input).run().get(0)) {
     tensor.dataToString()
}

Per https://github.com/tensorflow/java/issues/268#issuecomment-811663700 I was asked to only support eager mode. I suggest discussing this part with @rnett.

I think we should pattern the output after java.util.Arrays.deepToString(), which takes us back to brackets with commas.

float[][][] f = new float[][][] {
            {{1,2}, {3,4}},
            {{5, 6}, {7, 8}},
            {{9, 10}, {11, 12}}
};   
System.out.println(Arrays.deepToString(f));
// [[[1.0, 2.0], [3.0, 4.0]], [[5.0, 6.0], [7.0, 8.0]], [[9.0, 10.0], [11.0, 12.0]]]

My 2 cents: Make the default output human-friendly (i.e. square brackets as TF.print() and others do) and provide a Builder option that will request Java-style output. I find the indentation I provided above easier to read (for humans) than Java code, but I agree that the latter is useful functionality.

cowwoc avatar Apr 13 '21 19:04 cowwoc

Per #268 (comment) I was asked to only support eager mode. I suggest discussing this part with @rnett.

That's for Operands, Tensors should work fine regardless (which is what was in Jim's example).

My 2 cents: Make the default output human-friendly (i.e. square brackets as TF.print() and others do) and provide a Builder option that will request Java-style output. I find the indentation I provided above easier to read (for humans) than Java code, but I agree that the latter is useful functionality.

I'd agree with this: default to tf.print style but have options. I don't find the default Java style to be very readable for large tensors.

rnett avatar Apr 13 '21 20:04 rnett

@rnett @cowwoc FYI, in Graph mode, accessing Operand.asTensor() will fail without calling the session fetch first.

JimClarke5 avatar Apr 13 '21 20:04 JimClarke5

@rnett @cowwoc FYI, in Graph mode, accessing Operand.asTensor() will fail without calling the session fetch first.

Yeah, that's why we didn't have dataToString() do anything in Graph mode. It just calls asTensor().dataToString(), so it will do the same thing as asTensor().

rnett avatar Apr 13 '21 20:04 rnett

@cowwoc Your branch is behind master and that is causing me build issues.

JimClarke5 avatar Apr 14 '21 15:04 JimClarke5

@JimClarke5 One sec, I'll merge master into it.

cowwoc avatar Apr 14 '21 15:04 cowwoc

@JimClarke5 Try now.

cowwoc avatar Apr 14 '21 15:04 cowwoc

Actually the print code is keying on inheriting from NDArray. It has nothing to do specifically with TType. That is why @karllessard ’s comment is throwing me off.

JimClarke5 avatar Apr 16 '21 00:04 JimClarke5

@JimClarke5 yeah maybe my comment wasn't clear. I was just pointing out that the dataToString() method could be added in TType instead of Tensor since only typed tensors are supported anyway. The dataToString() version for raw tensors is converting to a typed tensor before printing it, something the user could do directly if needed.

But I'm also fine leaving it there, in Tensor, if that makes things easier or if you think it make more sense.

karllessard avatar Apr 16 '21 01:04 karllessard

Any updates on this? I am very new to Tensorflow and I have no idea how to visualize simple output. This seems perfect but it's been a year an a half since the last comment.

ajs1998 avatar Oct 16 '22 05:10 ajs1998

@ajs1998 you can print the value of any operand (Operand) using TF debug operations, such as tf.print(tf.strings.stringFormat(tensors...)) for example.

But if you want to print the values of a Tensor object itself without iterating through it, then that is still lacking afaik and should probably be added to the ndarray module instead. Could be an interesting and easy first PR for anyone interested to contribute to the project!

karllessard avatar Oct 16 '22 15:10 karllessard