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

Minimum prediction length at translation time #496

Merged
merged 2 commits into from Jan 4, 2018

Conversation

Projects
None yet
2 participants
@pltrdy
Contributor

pltrdy commented Jan 2, 2018

As we discussed in #457 we may want to force a minimum length for prediction by manually decreasing eos score for decoding step < min_length.

I set the option default on 0.

@srush

This comment has been minimized.

Show comment
Hide comment
@srush

srush Jan 3, 2018

Contributor

This is great, thanks!

One minor thing. Can you change the command line argument to min_sent_length to match the other argument name. Otherwise looks good.

Contributor

srush commented Jan 3, 2018

This is great, thanks!

One minor thing. Can you change the command line argument to min_sent_length to match the other argument name. Otherwise looks good.

@pltrdy

This comment has been minimized.

Show comment
Hide comment
@pltrdy

pltrdy Jan 3, 2018

Contributor

In fact, I started writing min_sent_length but I found it too specific and confusing (what if the sequence isn't text? or multi-sentence ? hmm).

Maybe min_output_length or min_pred_length or something of this kind would be better?

Contributor

pltrdy commented Jan 3, 2018

In fact, I started writing min_sent_length but I found it too specific and confusing (what if the sequence isn't text? or multi-sentence ? hmm).

Maybe min_output_length or min_pred_length or something of this kind would be better?

@srush

This comment has been minimized.

Show comment
Hide comment
@srush

srush Jan 3, 2018

Contributor

I'm fine with min_length, but then change the other arg to make it consistent.

Contributor

srush commented Jan 3, 2018

I'm fine with min_length, but then change the other arg to make it consistent.

@srush srush merged commit 66e3c18 into OpenNMT:master Jan 4, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@pltrdy pltrdy deleted the pltrdy:min_length branch Jan 4, 2018

KateDivision pushed a commit to KateDivision/OpenNMT-py that referenced this pull request Jun 29, 2018

Merge pull request OpenNMT#496 from pltrdy/min_length
Minimum prediction length at translation time
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment