-
Notifications
You must be signed in to change notification settings - Fork 202
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
Created TensorFormat enum #191
base: master
Are you sure you want to change the base?
Conversation
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). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
I don't think we can look at PRs until the CLA has been signed, so that's what this is waiting on. |
@googlebot I signed it
др Зоран Шеварац | Zoran Ševarac, PhD
Катедра за софтверско инжењерство | Department of Software Engineering
Ванредни професор | Associate Professor
[Fakultet organizacionih nauka]
Универзитет у Београду | University of Belgrade
Факултет организационих наука | Faculty of Organizational Sciences
Јове Илића 154, Београд, Србија | Jove Ilića 154, Belgrade, Serbia
t: +381 11 3950 853
e: zoran.sevarac@fon.bg.ac.rs<mailto:zoran.sevarac@fon.bg.ac.rs>
w: http://www.fon.bg.ac.rs
…________________________________
From: google-cla[bot] <notifications@github.com>
Sent: Friday, January 22, 2021 2:37:36 PM
To: tensorflow/java
Cc: Zoran V. Ševarac; Author
Subject: Re: [tensorflow/java] Created TensorFormat enum (#191)
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).
📝 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
* It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data<https://cla.developers.google.com/clas> and verify that your email is set on your git commits<https://help.github.com/articles/setting-your-email-in-git/>.
Corporate signers
* Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot<http://go/cla#troubleshoot> (Public version<https://opensource.google/docs/cla/#troubleshoot>).
* The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data<https://cla.developers.google.com/clas> and verify that your email is set on your git commits<https://help.github.com/articles/setting-your-email-in-git/>.
* The email used to register you as an authorized contributor must also be attached to your GitHub account<https://github.com/settings/emails>.
ℹ️ Googlers: Go here<https://goto.google.com/prinfo/https%3A%2F%2Fgithub.com%2Ftensorflow%2Fjava%2Fpull%2F191> for more info.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub<#191 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AABGTSKL2PBVDVJOWQRINRLS3F5SBANCNFSM4WOP2EMQ>.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we shouldn't add the other four enums as described in the link?
- CHWN 4d tensor description
- NCDHW 5d tensor description
- NDHWC
- CDHWN
The main question is are they are used by tensorflow at all? Maybe someone from Tensorflow team could answer this. |
@@ -0,0 +1,30 @@ | |||
package org.tensorflow.ndarray.buffer.layout; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @sevarac , as we discussed briefly during our last session, this enum should probably be moved at the tensorflow-core-api
or tensorflow-framework
level. It would be helpful to know which one if you can provide some quick examples of usage. I think @JimClarke5 had some in mind too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, in general everywhere where public Options dataFormat(String dataFormat) is used
there should be now public Options dataFormat(TensorFormat dataFormat)
which includes a bunch of classes mainly layers https://github.com/tensorflow/java/search?q=dataFormat
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is used in losses.Losses
for losses.CategoricalCrossentropy
and metrics.CategoricalCrossentropy
.
public static final int CHANNELS_LAST = -1;
public static final int CHANNELS_FIRST = 1;
Once this PR is merged, I'll change the logic in losses and metrics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be done in the C++ op generator, by looking at any argument called dataFormat
. Though I don't think this naming convention is enforced to the kernel developers, which might lead to mistakes. But there will be possible workarounds so I'm fine if you want to give a try making that change as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @sevarac , so any chance that you can move this enum to a different location, as I've suggested before?
If I need to pick one, I'll suggesttensorflow-framework
over tensorflow-core-api
, what about under org.tensorflow.framework.utils
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also please don't forget to add the header notice in your file (like this one, for example).
BTW: I will also need this in |
@@ -0,0 +1,30 @@ | |||
package org.tensorflow.ndarray.buffer.layout; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also please don't forget to add the header notice in your file (like this one, for example).
Created enum for TensorFormat. Some additional enum values are open for discussion, at the moment there are values that are used by Tensorflow and cuDnn