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

Chainer Unified encoder #629

Merged
merged 12 commits into from Mar 15, 2019

Conversation

@Fhrozen
Copy link
Contributor

commented Mar 8, 2019

Transcribing pytorch modification
Attending at #624

Fhrozen added 3 commits Mar 8, 2019
chainer Unified encoder
Transcribing pytorch modification

@Fhrozen Fhrozen changed the title [WIP] Chainer Unified encoder Chainer Unified encoder Mar 8, 2019

@Fhrozen

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

@sw005320 , Tested all encoders in AN4. Executed without problem.
Just finished the additional fixes, tried to clean up the configuration code for RNN. The code is quite similar to #624 proposed by @JaejinCho .
Let me know if there is any additional code to fix, or to discuss with @JaejinCho.

Fhrozen added 2 commits Mar 8, 2019
@sw005320

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

Can you prepare a test script for the unidirectional RNN?

@sw005320 sw005320 requested a review from JaejinCho Mar 8, 2019

@Fhrozen

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

Are the added test correct? should I add more? I tried to gather them by backend.

@sw005320
Copy link
Contributor

left a comment

Sorry for my late responses. I thought that I finished to put review comments, but it was pending.

('espnet.nets.chainer_backend.e2e_asr', 'blstmp', 'location', 'gru'),
('espnet.nets.chainer_backend.e2e_asr', 'bgrup', 'location', 'gru'),
('espnet.nets.chainer_backend.e2e_asr', 'lstmp', 'location', 'gru'),

This comment has been minimized.

Copy link
@sw005320

sw005320 Mar 13, 2019

Contributor
  • I think for this type of the unit test, we do not have to consider all "combinations" of attention and encoder/decoder types. If we prepare the all attention types given one encoder type (e.g., vggblstmp) and one decoder type (e.g., lstm), all encoder and decoder types given one attention type (e.g., location) , which is good enough. @kan-bayashi, what do you think?
  • it would be great if you add similar tests for pytorch and cleanup the order of this test a little bit.
@sw005320

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

@JaejinCho, could you also review this PR?

@Fhrozen Fhrozen changed the title Chainer Unified encoder Chainer Unified encoder and travis fix Mar 14, 2019

@Fhrozen

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2019

I modified the tests according as @sw005320 proposed. Let me know if this fits.
I also adding a fix about travis. Seems that the files cannot be downloaded from drive without confirmation.
I used this reference

@sw005320

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2019

Can you add test examples of unidirectional lstm/gru with VGG (e.g., vgglstm and vgglstmp) for both chainer and pytorch?

@Fhrozen

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2019

@sw005320 , I am not sure why travis is giving error, but when I test pytest in my env with conda I have no problem. is it required to add the compatibility test?. This two are the only problem.

@sw005320

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2019

We noticed this issue. You can ignore them for now.
@kan-bayashi, do you have any idea?

@kan-bayashi

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2019

I made PR to fix this issue (#649).
Download from google drive is a little bit difficult...

sw005320 added 2 commits Mar 14, 2019

@Fhrozen Fhrozen changed the title Chainer Unified encoder and travis fix Chainer Unified encoder Mar 14, 2019

@JaejinCho
Copy link
Contributor

left a comment

minor

super(RNNP, self).__init__()
bidir = typ[0] == "b"
if bidir:
linear_conn = L.NStepBiLSTM if "lstm" in typ else L.NStepBiGRU

This comment has been minimized.

Copy link
@JaejinCho

JaejinCho Mar 15, 2019

Contributor

I am personally not comfortable with this naming (linear_conn). why don't we use rnn? It doesn't seem to be used other than when you setattr w/ the index.

This comment has been minimized.

Copy link
@Fhrozen

Fhrozen Mar 15, 2019

Author Contributor

ty for the revision, I just modified as proposed. =)

@sw005320 sw005320 merged commit cf936f8 into espnet:master Mar 15, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sw005320

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

thanks!

@Fhrozen Fhrozen deleted the Fhrozen:uni_encoder-chainer branch Mar 15, 2019

@JaejinCho
Copy link
Contributor

left a comment

@sw005320 Sorry for telling this after merge but just "lstm" and "gru" for encoder in test are also possible although those usages might not be frequent. I guess I need to create a new PR again for it if needed since @Fhrozen deleted the developing branch already. Let me know

@Fhrozen

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

let me know about that, but since vgg test are included (+ lstm/gru) should not that be enough??.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.