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

Bug in initializer uniform_unit_scaling #2239

Closed
SivilTaram opened this Issue Dec 26, 2018 · 2 comments

Comments

Projects
None yet
3 participants
@SivilTaram
Copy link
Contributor

SivilTaram commented Dec 26, 2018

Describe the bug
bug in allennlp.nn.initializers.uniform_unit_scaling

To Reproduce
Using the latest version of allennlp and pytorch(0.4.1), initialize parameters of LSTM/GRU via uniform_unit_scaling function

Expected behavior
Exception throw:

RuntimeError: a leaf Variable that requires grad has been used in an in-place operation

Potential Solution
Fix the line to return tensor.data.uniform_(-max_value, max_value) link

@schmmd

This comment has been minimized.

Copy link
Member

schmmd commented Jan 3, 2019

@DeNeutoy does this look like a good fix to you? You added this code in #640.

@matt-gardner

This comment has been minimized.

Copy link
Member

matt-gardner commented Jan 3, 2019

@DeNeutoy is out for another little while; yes, this looks like a good fix. PR welcome.

SivilTaram added a commit to SivilTaram/allennlp that referenced this issue Jan 4, 2019

SivilTaram added a commit to SivilTaram/allennlp that referenced this issue Jan 4, 2019

Merge pull request #1 from SivilTaram/SivilTaram-patch-bug-fix-unifor…
…m_unit_scaling

Fix bug in uniform_unit_scaling allenai#2239

matt-gardner added a commit that referenced this issue Jan 4, 2019

WrRan added a commit to WrRan/allennlp that referenced this issue Jan 6, 2019

DeNeutoy added a commit that referenced this issue Jan 31, 2019

More help info for `archive_surgery.py` (#2327)
* Fix bug in uniform_unit_scaling #2239 (#2273)

* Fix type annotation for .forward(...) in tutorial (#2122)

* Add a Contributions section to README.md (#2277)

* script for doing archive surgery (#2223)

* script for doing archive surgery

* simplify script

* Fix spelling in tutorial README (#2283)

* fix #2285 (#2286)

* Update the `find-lr` subcommand help text. (#2289)

* Update the elmo command help text.

* Update the find-lr subcommand help text.

* Add __repr__ to Vocabulary (#2293)

As it currently stands, the following is logged during training:

```
2019-01-06 10:46:21,832 - INFO - allennlp.common.from_params - instantiating class <class 'allennlp.model
s.language_model.LanguageModel'> from params {'bidirectional': False, 'contextualizer': {'bidirectional':
 False, 'dropout': 0.5, 'hidden_size': 200, 'input_size': 200, 'num_layers': 2, 'type': 'lstm'}, 'dropout
': 0.5, 'text_field_embedder': {'token_embedders': {'tokens': {'embedding_dim': 200, 'type': 'embedding'}
}}} and extras {'vocab': <allennlp.data.vocabulary.Vocabulary object at 0x7ff7811665f8>}
```

Note that the `Vocabulary` does not provide any useful information, since it doesn't have `__repr__` defined. This provides a fix.

* Update the base image in the Dockerfiles. (#2298)

* Don't deprecate bidirectional-language-model name (#2297)

* bump version number to v0.8.1

* Bump version numbers to v0.8.2-unreleased

* Turn BidirectionalLM into a more-general LanguageModel class (#2264)

Fixes #2255

This PR replaces the `BidirectionalLM` class with a more-general `LanguageModel` that can be used in either the unidirectional/forward setting or the bidirectional setting.

It also accordingly replaces the `BidirectionalLanguageModelTokenEmbedder` with a `LanguageModelTokenEmbedder`.

Also fixes bug in the experiment_unsampled.jsonnet config that was preventing a test from actually being unsampled.

TODO:

- [x] test the unidirectional case
- [x] properly deprecate `BidirectionalLM` and `BidirectionalLanguageModelTokenEmbedder` 
- [x] check docs for accuracy
- [x] fix user-facing training configs

* more help info

* typo fix

* add option '--inplace', '--force'

* clearer help text

matt-gardner added a commit that referenced this issue Mar 18, 2019

Move some scripts to `allennlp/allennlp/tools` (#2584)
* Fix bug in uniform_unit_scaling #2239 (#2273)

* Fix type annotation for .forward(...) in tutorial (#2122)

* Add a Contributions section to README.md (#2277)

* script for doing archive surgery (#2223)

* script for doing archive surgery

* simplify script

* Fix spelling in tutorial README (#2283)

* fix #2285 (#2286)

* Update the `find-lr` subcommand help text. (#2289)

* Update the elmo command help text.

* Update the find-lr subcommand help text.

* Add __repr__ to Vocabulary (#2293)

As it currently stands, the following is logged during training:

```
2019-01-06 10:46:21,832 - INFO - allennlp.common.from_params - instantiating class <class 'allennlp.model
s.language_model.LanguageModel'> from params {'bidirectional': False, 'contextualizer': {'bidirectional':
 False, 'dropout': 0.5, 'hidden_size': 200, 'input_size': 200, 'num_layers': 2, 'type': 'lstm'}, 'dropout
': 0.5, 'text_field_embedder': {'token_embedders': {'tokens': {'embedding_dim': 200, 'type': 'embedding'}
}}} and extras {'vocab': <allennlp.data.vocabulary.Vocabulary object at 0x7ff7811665f8>}
```

Note that the `Vocabulary` does not provide any useful information, since it doesn't have `__repr__` defined. This provides a fix.

* Update the base image in the Dockerfiles. (#2298)

* Don't deprecate bidirectional-language-model name (#2297)

* bump version number to v0.8.1

* Bump version numbers to v0.8.2-unreleased

* Turn BidirectionalLM into a more-general LanguageModel class (#2264)

Fixes #2255

This PR replaces the `BidirectionalLM` class with a more-general `LanguageModel` that can be used in either the unidirectional/forward setting or the bidirectional setting.

It also accordingly replaces the `BidirectionalLanguageModelTokenEmbedder` with a `LanguageModelTokenEmbedder`.

Also fixes bug in the experiment_unsampled.jsonnet config that was preventing a test from actually being unsampled.

TODO:

- [x] test the unidirectional case
- [x] properly deprecate `BidirectionalLM` and `BidirectionalLanguageModelTokenEmbedder` 
- [x] check docs for accuracy
- [x] fix user-facing training configs

* move some utilities from allennlp/scripts to allennlp/allennlp/tools

* make pylint happy

* add modules to API doc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.