-
Couldn't load subscription status.
- Fork 219
Created TensorFormat enum #191
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
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 @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 and verify that your email is set on your git commits.
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 (Public version).
- The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
- The email used to register you as an authorized contributor must also be attached to your GitHub account.
i️ 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.
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.
I've found these layouts in cuDNN documentation so there is good chance that they are used.
It would be best to add them too if they are used, otherwise not sure if it makes sense to add them
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 layers.
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