This repository has been archived by the owner on Nov 22, 2022. It is now read-only.
Minor refactor and code rearrange on module.py #1576
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D25997360 |
mikekgfb
force-pushed
the
export-D25997360
branch
from
0154c5e
to
edf1f3d
Compare
|
This pull request was exported from Phabricator. Differential Revision: D25997360 |
mikekgfb
added a commit
to mikekgfb/pytext
that referenced
this pull request
Jan 23, 2021
Summary: Pull Request resolved: facebookresearch#1576 Refactor module.py with Goal to share methods between the ScriptPytextEmbeddingModule and ScriptPytextModule 1 - moved embedding module to top of file 2 - made pytext module depend on embedding module (pytext module is embedding module + 1 output layer) 3 - collapsed ScriptModule with a single method into embedding module. Collapsing ScriptModule is possible after the refactor because now all modules are in a single inheritance tree, which allows them to reuse each others methods, so we can share set_device, and don't have to create an artificial ancestor to just share set_device(). New hierarchy after refactor reflects that a (classifier) "module" is an embedding module followed by an output layer: ``` EmbeddingModule / \ / Embedding Module with dense / \ (Classifier) Module (Classifier) Module with dense ``` Differential Revision: D25997360 fbshipit-source-id: 8a73a4a2cae4de2034d8552346071b6c3a887172
mikekgfb
force-pushed
the
export-D25997360
branch
from
edf1f3d
to
275e7fb
Compare
|
This pull request was exported from Phabricator. Differential Revision: D25997360 |
1 similar comment
|
This pull request was exported from Phabricator. Differential Revision: D25997360 |
mikekgfb
added a commit
to mikekgfb/pytext
that referenced
this pull request
Feb 11, 2021
Summary: Pull Request resolved: facebookresearch#1576 Refactor module.py with Goal to share methods between the ScriptPytextEmbeddingModule and ScriptPytextModule 1 - moved embedding module to top of file 2 - made pytext module depend on embedding module (pytext module is embedding module + 1 output layer) 3 - collapsed ScriptModule with a single method into embedding module. Collapsing ScriptModule is possible after the refactor because now all modules are in a single inheritance tree, which allows them to reuse each others methods, so we can share set_device, and don't have to create an artificial ancestor to just share set_device(). New hierarchy after refactor reflects that a (classifier) "module" is an embedding module followed by an output layer: ``` EmbeddingModule / \ / Embedding Module with dense / \ (Classifier) Module (Classifier) Module with dense ``` Differential Revision: D25997360 fbshipit-source-id: d7be139e8a0e76de52ab60bae2ab0eb43b867ab9
mikekgfb
force-pushed
the
export-D25997360
branch
from
275e7fb
to
e9a8e53
Compare
|
This pull request was exported from Phabricator. Differential Revision: D25997360 |
mikekgfb
added a commit
to mikekgfb/pytext
that referenced
this pull request
Feb 11, 2021
Summary: Pull Request resolved: facebookresearch#1576 Refactor module.py with Goal to share methods between the ScriptPytextEmbeddingModule and ScriptPytextModule 1 - moved embedding module to top of file 2 - made pytext module depend on embedding module (pytext module is embedding module + 1 output layer) 3 - collapsed ScriptModule with a single method into embedding module. Collapsing ScriptModule is possible after the refactor because now all modules are in a single inheritance tree, which allows them to reuse each others methods, so we can share set_device, and don't have to create an artificial ancestor to just share set_device(). New hierarchy after refactor reflects that a (classifier) "module" is an embedding module followed by an output layer: ``` EmbeddingModule / \ / Embedding Module with dense / \ (Classifier) Module (Classifier) Module with dense ``` Differential Revision: D25997360 fbshipit-source-id: 0c48095727a817a174fb7330bde936541d38634b
mikekgfb
force-pushed
the
export-D25997360
branch
from
e9a8e53
to
d2d9ce2
Compare
|
This pull request was exported from Phabricator. Differential Revision: D25997360 |
mikekgfb
added a commit
to mikekgfb/pytext
that referenced
this pull request
Feb 12, 2021
Summary: Pull Request resolved: facebookresearch#1576 Refactor module.py with Goal to share methods between the ScriptPytextEmbeddingModule and ScriptPytextModule 1 - moved embedding module to top of file 2 - made pytext module depend on embedding module (pytext module is embedding module + 1 output layer) 3 - collapsed ScriptModule with a single method into embedding module. Collapsing ScriptModule is possible after the refactor because now all modules are in a single inheritance tree, which allows them to reuse each others methods, so we can share set_device, and don't have to create an artificial ancestor to just share set_device(). New hierarchy after refactor reflects that a (classifier) "module" is an embedding module followed by an output layer: ``` EmbeddingModule / \ / Embedding Module with dense / \ (Classifier) Module (Classifier) Module with dense ``` Differential Revision: D25997360 fbshipit-source-id: a1e81b7ed1f152d445065808d0ca806658f5251b
mikekgfb
force-pushed
the
export-D25997360
branch
from
d2d9ce2
to
bf8f8f1
Compare
|
This pull request was exported from Phabricator. Differential Revision: D25997360 |
mikekgfb
added a commit
to mikekgfb/pytext
that referenced
this pull request
Feb 14, 2021
Summary: Pull Request resolved: facebookresearch#1576 Refactor module.py with Goal to share methods between the ScriptPytextEmbeddingModule and ScriptPytextModule 1 - moved embedding module to top of file 2 - made pytext module depend on embedding module (pytext module is embedding module + 1 output layer) 3 - collapsed ScriptModule with a single method into embedding module. Collapsing ScriptModule is possible after the refactor because now all modules are in a single inheritance tree, which allows them to reuse each others methods, so we can share set_device, and don't have to create an artificial ancestor to just share set_device(). New hierarchy after refactor reflects that a (classifier) "module" is an embedding module followed by an output layer: ``` EmbeddingModule / \ / Embedding Module with dense / \ (Classifier) Module (Classifier) Module with dense ``` Differential Revision: D25997360 fbshipit-source-id: c263566fe89c72ef0828436bfb71fe0244e568fb
mikekgfb
force-pushed
the
export-D25997360
branch
from
bf8f8f1
to
15bad40
Compare
Summary: Pull Request resolved: facebookresearch#1576 Refactor module.py with Goal to share methods between the ScriptPytextEmbeddingModule and ScriptPytextModule 1 - moved embedding module to top of file 2 - made pytext module depend on embedding module (pytext module is embedding module + 1 output layer) 3 - collapsed ScriptModule with a single method into embedding module. Collapsing ScriptModule is possible after the refactor because now all modules are in a single inheritance tree, which allows them to reuse each others methods, so we can share set_device, and don't have to create an artificial ancestor to just share set_device(). New hierarchy after refactor reflects that a (classifier) "module" is an embedding module followed by an output layer: ``` EmbeddingModule / \ / Embedding Module with dense / \ (Classifier) Module (Classifier) Module with dense ``` Differential Revision: D25997360 fbshipit-source-id: f3027a0c43de15938288876eedec2d2977ea879c
mikekgfb
force-pushed
the
export-D25997360
branch
from
15bad40
to
2f67bed
Compare
|
This pull request was exported from Phabricator. Differential Revision: D25997360 |
|
This pull request has been merged in 22cb76a. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
Minor refactor and code rearrange on module.py
Goal is to reuse the Pytext embedding module methods for pytext module
Differential Revision: D25997360