learnable_param_ids_ fix #2909
Merged
learnable_param_ids_ fix #2909
Conversation
This file contains hidden or 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
Contributor
Author
|
Merging after discussion with @shelhamer and @longjon as this is a bugfix that may be causing user issues. I should eventually add unit tests as noted in #2910. |
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
In #2866 I added
vector<int> learnable_param_ids_, which was supposed to be a mapping fromparams_indices tolearnable_params_indices used to check thatlr_multanddecay_mults matched up. I screwed this up by only appending IDs of owner params, and forgetting to add IDs for sharing (non-owning) params. I think the only resulting problem (given thatlearnable_param_ids_is only read in one place) is that you could run into false positives or false negatives with the check that{lr,decay}_mults of shared parameters are equal. Very sorry for the trouble if this happened to you.Thanks to @Otkrist for reporting this issue! (He was using shared params and ran into a the
lr_mult mismatchcheck failure when using the samelr_multfor all params. He verified that this patch fixes the issue for him.)