[BUGFIX] Fix _get_rnn_cell

[MarisaKirisame]

No description provided.

[mli]

Job PR-648/1 is complete.
Docs are uploaded to http://gluon-nlp-staging.s3-accelerate.dualstack.amazonaws.com/PR-648/1/index.html

[codecov]

Codecov Report

Merging #648 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master     #648   +/-   ##
=======================================
  Coverage   63.76%   63.76%           
=======================================
  Files         141      141           
  Lines       12870    12870           
=======================================
  Hits         8207     8207           
  Misses       4663     4663

Flag	Coverage Δ
#PR648	63.76% <100%> (?)
#master	?
#notserial	42.64% <100%> (ø)	⬆️
#py2	63.52% <100%> (ø)	⬆️
#py3	63.61% <100%> (ø)	⬆️
#serial	49.22% <100%> (ø)	⬆️

[codecov]

Codecov Report

Merging #648 into master will increase coverage by 0.1%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master     #648     +/-   ##
=========================================
+ Coverage   90.94%   91.04%   +0.1%     
=========================================
  Files          64       64             
  Lines        5887     5887             
=========================================
+ Hits         5354     5360      +6     
+ Misses        533      527      -6

Impacted Files	Coverage Δ
src/gluonnlp/model/utils.py	76.72% <100%> (ø)	⬆️
src/gluonnlp/data/dataloader.py	88.79% <0%> (+5.17%)	⬆️

[eric-haibin-lin]

Thanks for the contribution!

[cgraywang]

Nice catch! proj_size is required for lstmpc. One simple modification would be removing mode == lstmpc, and add a condition if mode == 'lstmpc'.

[eric-haibin-lin]

Sorry in the last review I didn't realize this change will break lstmpc cell. What about moving the assertion inside LSTMPCellWithClip to check proj_size, cell_clip and proj_clip ?

gluon-nlp/src/gluonnlp/model/lstmpcellwithclip.py

Lines 85 to 97 in aa2c7e6

	super(LSTMPCellWithClip, self).__init__(hidden_size,
	projection_size,
	i2h_weight_initializer,
	h2h_weight_initializer,
	h2r_weight_initializer,
	i2h_bias_initializer,
	h2h_bias_initializer,
	input_size,
	prefix=prefix,
	params=params)
	self._cell_clip = cell_clip
	self._projection_clip = projection_clip

[eric-haibin-lin]

Sorry in the last review I didn't realize this change will break lstmpc cell. What about moving the assertion inside LSTMPCellWithClip to check proj_size, cell_clip and proj_clip ?

gluon-nlp/src/gluonnlp/model/lstmpcellwithclip.py

Lines 85 to 97 in aa2c7e6

	super(LSTMPCellWithClip, self).__init__(hidden_size,
	projection_size,
	i2h_weight_initializer,
	h2h_weight_initializer,
	h2r_weight_initializer,
	i2h_bias_initializer,
	h2h_bias_initializer,
	input_size,
	prefix=prefix,
	params=params)
	self._cell_clip = cell_clip
	self._projection_clip = projection_clip

[szha]

Thanks for the fix, @MarisaKirisame. Would you have some time to address the comments? Let me know how I can help.

[MarisaKirisame]

@szha hi, sorry for the delay! I was pushing for a conference deadline for a week (this come up). Now that it is over I can actually write other code. I will look at this and fix ASAP.

[mli]

Job PR-648/3 is complete.
Docs are uploaded to http://gluon-nlp-staging.s3-accelerate.dualstack.amazonaws.com/PR-648/3/index.html

[mli]

Job PR-648/2 is complete.
Docs are uploaded to http://gluon-nlp-staging.s3-accelerate.dualstack.amazonaws.com/PR-648/2/index.html

[MarisaKirisame]

@eric-haibin-lin I cannot move them inside.
The problem is, I have to check that if lstmpc is on, it is not None, and if it is off it is None.
The latter must be done outside of LSTMPCellWithClip.
IMO the principle way to fix this is to let _get_rnn_cell take a cell instead of take a mode.
What do you think?

[szha]

@eric-haibin-lin ping.

[eric-haibin-lin]

@MarisaKirisame sorry for the late reply. If a cell is passed, we still need to contruct a cell somewhere. What about just fixing the assertion in the following way?

    if mode != 'lstmpc':
        assert proj_size is None, 'proj_size takes effect only when mode is lstmpc'
        assert cell_clip is None, 'cell_clip takes effect only when mode is lstmpc'
        assert proj_clip is None, 'proj_clip takes effect only when mode is lstmpc'

[MarisaKirisame]

@eric-haibin-lin that is what the current fix do, it is just in different form. Is there reason to prefer one over the other?

[eric-haibin-lin]

nvm. The current fix looks good. Just triggered the CI again. Thanks for the fix!

[mli]

Job PR-648/2 is complete.
Docs are uploaded to http://gluon-nlp-staging.s3-accelerate.dualstack.amazonaws.com/PR-648/2/index.html

proposed alternative is equivalent