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

Include micro averaging - Bleu Score #2179

Merged
merged 21 commits into from
Sep 7, 2021
Merged

Conversation

Ishan-Kumar2
Copy link
Contributor

@Ishan-Kumar2 Ishan-Kumar2 commented Aug 26, 2021

Fixes #2178

Description:
Have included method to pass batch of inputs to update and allow macro averaging also.
[WIP] The tests need to be added

Check list:

  • New tests are added (if a new feature is added)
  • New doc strings: description and/or example code are in RST format
  • Documentation is updated (if required)

@github-actions github-actions bot added the module: metrics Metrics module label Aug 26, 2021
@Ishan-Kumar2
Copy link
Contributor Author

@sdesrozis could you have a look, if the structure is correct so that I can change the tests also.

@sdesrozis
Copy link
Contributor

@sdesrozis could you have a look, if the structure is correct so that I can change the tests also.

@Ishan-Kumar2 I did a plenty of comments, sorry for that. I think we could improve a bit what you did. The main point is the use of local/class variables.

@sdesrozis
Copy link
Contributor

Another remark. Micro and batch are two things which could be tackled separately. If you loop over the batch example, you can have a batch macro version (which is not done).

Micro average is the "correct" (I mean most common) bleu metric. This is naturally compatible with batch.

I suggest that we keep the actual update api. The batch should be added in another PR, what do you think ?

@Ishan-Kumar2
Copy link
Contributor Author

@sdesrozis thank you for the helpful reviews, I will first make another PR with just the addition of batch (and corresponding tests) then continue here for micro.

@sdesrozis
Copy link
Contributor

Do as you prefer. I can help on the development of this PR because it is very clear to me 😉 Let me know.

@Ishan-Kumar2
Copy link
Contributor Author

@sdesrozis , I have merged the other PRs contents and made the changes you suggested.
I have retained the _corpus_bleu function for now, as it is used in the tests. Do you think having a wrapper function for calculating bleu directly is useful? Otherwise one would have to call both _n_gram_counter and _brevity_penalty_smoothing_macro if they want to quickly check bleu for 1 example. I can rename it _corpus_bleu_macro if we decide to keep it.

If the micro avg calculation method is correct, I can add tests for the same.

@Ishan-Kumar2 Ishan-Kumar2 changed the title Include micro averaging on batch - Bleu Score Include micro averaging - Bleu Score Sep 1, 2021
@sdesrozis sdesrozis marked this pull request as draft September 2, 2021 07:14
@sdesrozis
Copy link
Contributor

I converted to draft since it misses the tests and maybe some discussion is still needed.

@sdesrozis
Copy link
Contributor

@Ishan-Kumar2 Thanks a lot for the work, although a few last comments to address.

Moreover, the tests seems ko.

@Ishan-Kumar2 Ishan-Kumar2 marked this pull request as ready for review September 7, 2021 10:35
Copy link
Contributor

@sdesrozis sdesrozis left a comment

Choose a reason for hiding this comment

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

@Ishan-Kumar2 Thank you very much ! LGTM !

Copy link
Collaborator

@vfdev-5 vfdev-5 left a comment

Choose a reason for hiding this comment

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

A nit comment, otherwise LGTM as well

ignite/metrics/nlp/bleu.py Outdated Show resolved Hide resolved
@vfdev-5 vfdev-5 enabled auto-merge (squash) September 7, 2021 16:14
@vfdev-5 vfdev-5 merged commit 4a243a1 into pytorch:master Sep 7, 2021
@Ishan-Kumar2 Ishan-Kumar2 deleted the update_bleu branch September 7, 2021 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: metrics Metrics module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using Bleu for batch input
3 participants