DO NOT SUBMIT : String tensor #257
Conversation
…KnownVocab and RainbowVocab
|
This branch is an exploration of how preprocessing layers might look. It is not intended for submission. Created this PR to enable easier sharing with interested parties. |
|
Just a quick question before more thorough reviewing: why implement Review status: 0 of 1 LGTMs obtained Comments from Reviewable |
|
We may wish graduate StringTensor from tfjs-layers to tfjs-core, if it
proves useful in that context. I explored this option, and there are a few
reasons why making it an actual Tensor by extending DataType is problematic.
1. Tensor operations always defer to the ENV for actual computation, but
we don't anticipate StringTensor ever running on GPU. It is not currently
possible to have more than one ENV in one tfjs application. This
complicates the code for, e.g., Tensor.slice. and Tensor.reshape.
2. Tensor includes many API calls for math-like operations
(myTensor.log()). Which would need to throw 'NotImplementedError' or
something for these operations on Tensor with dtype string.
3. The three existing DataTypes are roughly suitable for math-like
operations. They are somewhat compatible. Adding 'string' here would
require adding 'not-implemented' for many of the math-like operations in
core.
Given that StringTensor is unlikely to be involved in any
back-propagation-like computations, I think having StringTensor be part of
the high-level library, apart from tfjs-core, is defensible.
That said, there are several clear weaknesses of the approach in this PR,
as it is currently stands. Having StringTensor apart from Tensor makes
some of the API calls in the layers library more cumbersome, especially WRT
to the types of arguments and return values. This may be addressed
eventually by having a higher level ancestor to both, but it's not clear at
this point exactly what capabilities, like slice() and reshape(), that
ancestor must expose.
…On Fri, Jun 29, 2018 at 8:58 PM, Shanqing Cai ***@***.***> wrote:
Just a quick question before more thorough reviewing: why implement
StringTensor in tfjs-layers? Shouldn't it be in tfjs-core and just be an
additional dtype of the existing Tensor class?
------------------------------
Review status: 0 of 1 LGTMs obtained
------------------------------
*Comments from Reviewable
<https://reviewable.io/reviews/tensorflow/tfjs-layers/257#-:-LGDBL_Z7L2hLfz3VKTk:bxqtjp9>*
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#257 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAhZTrpFF7LWWRL0w7zc26tbpvLXNmMzks5uBs0ZgaJpZM4U9nvp>
.
--
Stan Bileschi Ph.D. | SWE | bileschi@google.com | 617-230-8081
|
|
Some tests in src/engine/preprocessing_training.ts intend to really train a model with a vocabulary layer, going beyond what fitUnsupervised does. What should the trainable weight variables be, since the methods relevant to strings in preprocess_core.ts appear to only involve vocabulary management? |
|
An afterthought which might answer the question. |
|
In the design that I am promoting, it will be possible to use the input data to adjust the VocabLayer's internal statistics. It is un settled at this point whether this will be an operation that is part of the PreproecessingLayer API, or whether it will be triggered by the model, or something else entirely. Interested to hear your thoughts and preferences on the matter! |
|
Thanks for your willingness to hear my thoughts on the matter. There are two things that come to mind now. I assume that the updating process of the vocabulary layer is needed in a situation where a pre-trained model has been imported and should be re-trained, but in the new data there are vocabulary items which were not in the original data. A first consideration is that this would also require that some imported weights in the model be augmented in size, and the enlarged weights would have to be initialised at the new points. A second consideration is that what is imported typically consists not only of pre-trained model weights, but also so-called word embeddings like the GloVe pre-trained vectors, so there would also need to be a process for assigning embeddings to new vocabulary. These are two situations which I remember encountering. Maybe accommodating them would be desiderata for typical users of the software. |
|
This PR is superseded by the addition of string tensors in @tensorflow/tfjs-core #1408 |
|
Hello Stan,
I noticed via this mail in January that the string-tensor branch of
tfjs-layers was no longer being developed.
There is another, related stale branch called preproc-layer-vocab. Has the
approach initiated in this branch also been abandoned?
Regards,
Terry
…On Fri, 4 Jan 2019 at 17:12, Stanley Bileschi ***@***.***> wrote:
Closed #257 <#257>.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#257 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ATRCz_njxvolXcJkitttrvRqVnySbzL7ks5u_319gaJpZM4U9nvp>
.
|
|
Hi @tcboswell , Thanks for your interest! We decided to implement this another way. See here: This functionality hasn't been pulled through to layers yet, though, since there currently aren't any Keras layers which take string tensors (though this is changing with addition of Keras preprocessing layers) keras-team/governance@6eb818b#diff-759af1e95f3c3f6515f6287a1a786470R11 Is there something that you need tfjs-layers to do that wouldn't be covered by this? Sincerely, |
|
Thanks for asking, Stan, I cannot think of anything at the moment. The
proposal in the second link seems to contain the functionality I was hoping
for.
Do I understand rightly that the implementation of this in tensorflowjs is
on hold until it has been implemented in keras?
…On Mon, 17 Jun 2019 at 17:53, Stanley Bileschi ***@***.***> wrote:
Hi @tcboswell <https://github.com/tcboswell> ,
Thanks for your interest! We decided to implement this another way. See
here:
tensorflow/tfjs-core#1408
<tensorflow/tfjs-core#1408> (which Layers depends
on)
This functionality hasn't been pulled through to layers yet, though, since
there currently aren't any Keras layers which take string tensors (though
this is changing with addition of Keras preprocessing layers)
***@***.***#diff-759af1e95f3c3f6515f6287a1a786470R11
<keras-team/governance@6eb818b#diff-759af1e95f3c3f6515f6287a1a786470R11>
Is there something that you need tfjs-layers to do that wouldn't be
covered by this?
Sincerely,
Stan
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#257?email_source=notifications&email_token=AE2EFT5KAYUA5F7LWVYCYM3P26XN3A5CNFSM4FHWPPU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODX3TUTQ#issuecomment-502741582>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AE2EFT3K2QNN4BO73RIVTRLP26XN3ANCNFSM4FHWPPUQ>
.
|
|
Thats correct, though the version in Keras is in progress. See e.g.,
https://github.com/tensorflow/tensorflow/blob/master/tensorflow/python/keras/engine/base_preprocessing_layer.py
…On Tue, Jun 18, 2019 at 1:58 PM tcboswell ***@***.***> wrote:
Thanks for asking, Stan, I cannot think of anything at the moment. The
proposal in the second link seems to contain the functionality I was hoping
for.
Do I understand rightly that the implementation of this in tensorflowjs is
on hold until it has been implemented in keras?
On Mon, 17 Jun 2019 at 17:53, Stanley Bileschi ***@***.***>
wrote:
> Hi @tcboswell <https://github.com/tcboswell> ,
>
> Thanks for your interest! We decided to implement this another way. See
> here:
> tensorflow/tfjs-core#1408
> <tensorflow/tfjs-core#1408> (which Layers
depends
> on)
>
> This functionality hasn't been pulled through to layers yet, though,
since
> there currently aren't any Keras layers which take string tensors (though
> this is changing with addition of Keras preprocessing layers)
>
> ***@***.***#diff-759af1e95f3c3f6515f6287a1a786470R11
> <
keras-team/governance@6eb818b#diff-759af1e95f3c3f6515f6287a1a786470R11
>
>
> Is there something that you need tfjs-layers to do that wouldn't be
> covered by this?
>
> Sincerely,
> Stan
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <
#257?email_source=notifications&email_token=AE2EFT5KAYUA5F7LWVYCYM3P26XN3A5CNFSM4FHWPPU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODX3TUTQ#issuecomment-502741582
>,
> or mute the thread
> <
https://github.com/notifications/unsubscribe-auth/AE2EFT3K2QNN4BO73RIVTRLP26XN3ANCNFSM4FHWPPUQ
>
> .
>
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#257?email_source=notifications&email_token=AAEFSTSGOMP5CQSFNJ55EUDP3EO3ZA5CNFSM4FHWPPU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODX7OS7I#issuecomment-503245181>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEFSTWYV7JD25CJYB6JBC3P3EO3ZANCNFSM4FHWPPUQ>
.
--
Stan Bileschi Ph.D. | SWE | bileschi@google.com | 617-230-8081
|
Description
For repository owners only:
Please remember to apply all applicable tags to your pull request.
Tags: FEATURE, BREAKING, BUG, PERF, DEV, DOC, SECURITY
For more info see: https://github.com/tensorflow/tfjs/blob/master/DEVELOPMENT.md
This change is