nlp_seq to linen port#609
Conversation
Codecov Report
@@ Coverage Diff @@
## master #609 +/- ##
==========================================
- Coverage 80.44% 79.31% -1.14%
==========================================
Files 55 55
Lines 4199 4259 +60
==========================================
Hits 3378 3378
- Misses 821 881 +60
Continue to review full report at Codecov.
|
| This script trains a Transformer on the Universal dependency dataset. | ||
| """ | ||
|
|
||
| import tensorflow as tf |
There was a problem hiding this comment.
https://www.python.org/dev/peps/pep-0008/#imports
Imports should be grouped in the following order:
1. Standard library imports.
2. Related third party imports.
3. Local application/library specific imports.
You should put a blank line between each group of imports.
| # Do nothing in predict mode, as then the sequence length is 1. | ||
| return x | ||
|
|
||
| from flax import linen as nn |
There was a problem hiding this comment.
https://www.python.org/dev/peps/pep-0008/#imports
Imports should be grouped in the following order:
1. Standard library imports.
2. Related third party imports.
3. Local application/library specific imports.
You should put a blank line between each group of imports.
There was a problem hiding this comment.
Thanks! Changed tool
| dtype=cfg.dtype, | ||
| kernel_init=cfg.kernel_init, | ||
| bias_init=cfg.bias_init)(inputs) | ||
| x = nn.relu(x) |
There was a problem hiding this comment.
gelu -> relu : is this intentional ?
There was a problem hiding this comment.
no not intentional. changed it back.
| @nn.compact | ||
| def __call__(self, | ||
| inputs, | ||
| inputs_positions=None): |
There was a problem hiding this comment.
(Here and elsewhere)
Can we make all arguments to __call__() kw-only?
Ideally, we would also add a type annotation.
See also #231 (comment)
There was a problem hiding this comment.
done. left type annotations out (for now?)
| train=True, | ||
| dropout_rate=0.3, | ||
| attention_dropout_rate=0.3): | ||
| train=False, |
There was a problem hiding this comment.
Can we make the additional arguments kw-only and remove the default from the train argument?
See also #231 (comment)
| dropout_rate=0.3, | ||
| attention_dropout_rate=0.3): | ||
| train=False, | ||
| dropout_rate=0.3): |
There was a problem hiding this comment.
Why is this not a setting in TransformerConfig ?
There was a problem hiding this comment.
now taken from TransformerConfig. Thanks!
There was a problem hiding this comment.
use dropout from config.
| @nn.compact | ||
| def __call__(self, | ||
| inputs, | ||
| encoder_mask=None, |
There was a problem hiding this comment.
can we make all arguments to __call__() kw-only?
See also #231 (comment)
| lambda x: x / eval_denominator, # pylint: disable=cell-var-from-loop | ||
| eval_metrics_sums) | ||
|
|
||
| # Calculate (clipped) perplexity after averaging log-perplexities: |
Changes due to review of andsteing no resolved PR #609
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #609 +/- ##
==========================================
- Coverage 80.44% 79.31% -1.14%
==========================================
Files 55 55
Lines 4199 4259 +60
==========================================
Hits 3378 3378
- Misses 821 881 +60 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
4 participants
I carried out the changes to port the example to linen.