Skip to content
This repository has been archived by the owner on Dec 16, 2022. It is now read-only.

Fix a bug in the last decoding step of training CopyNet #296

Merged
merged 3 commits into from
Aug 19, 2021

Conversation

GregoryZeng
Copy link
Contributor

@GregoryZeng GregoryZeng commented Aug 18, 2021

Copy link
Member

@epwalsh epwalsh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @GregoryZeng! This is great. I think the comment on lines 495-496 is still confusing:

        # The last input from the target is either padding or the end symbol.
        # Either way, we don't have to process it.
        num_decoding_steps = target_sequence_length - 1

I think it would be more clear if it said something like:

        # We have a decoding step for every target token except the "START" token.
        num_decoding_steps = target_sequence_length - 1

Do you agree? If so, could you update that in this PR as well?

@epwalsh epwalsh self-assigned this Aug 19, 2021
@GregoryZeng
Copy link
Contributor Author

Sure, that's definitely clearer.

Copy link
Member

@epwalsh epwalsh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thanks @GregoryZeng!

@epwalsh epwalsh enabled auto-merge (squash) August 19, 2021 17:54
@epwalsh epwalsh merged commit 8d5b21f into allenai:main Aug 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

the last decoding step miscalculated in the forward_loss of CopyNet?
2 participants