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

Fixed logprobs for greedy. #886

Merged
merged 2 commits into from
Sep 20, 2024

Conversation

popovaan
Copy link
Contributor

Applied LogSoftmax to logits returned by _greedy_sample().
Ticket: CVS-152274

@ilya-lavrenov ilya-lavrenov self-assigned this Sep 20, 2024
@ilya-lavrenov ilya-lavrenov added this to the 2024.5 milestone Sep 20, 2024
@popovaan
Copy link
Contributor Author

popovaan commented Sep 20, 2024

I checked multinomial and it returns logprobs in range in [0,1], expected (-inf, 0].
So maybe add logsoftmax to LogitTransformers so it applied for both greedy and multinomial?
https://github.com/openvinotoolkit/openvino.genai/blob/master/src/cpp/src/logit_processor.hpp#L325-L355

@dtrawins
Copy link
Contributor

I checked multinomial and it returns logprobs in range in [0,1], expected (-inf, 0]. So maybe add logsoftmax to LogitTransformers so it applied for both greedy and multinomial? https://github.com/openvinotoolkit/openvino.genai/blob/master/src/cpp/src/logit_processor.hpp#L325-L355

Indeed it would be the best to include log10 operation to the current prob values so the returned results would match the logprobs property name.

@popovaan
Copy link
Contributor Author

I checked multinomial and it returns logprobs in range in [0,1], expected (-inf, 0]. So maybe add logsoftmax to LogitTransformers so it applied for both greedy and multinomial? https://github.com/openvinotoolkit/openvino.genai/blob/master/src/cpp/src/logit_processor.hpp#L325-L355

Indeed it would be the best to include log10 operation to the current prob values so the returned results would match the logprobs property name.

Done, please check.

@ilya-lavrenov ilya-lavrenov added this pull request to the merge queue Sep 20, 2024
Merged via the queue into openvinotoolkit:master with commit de77f96 Sep 20, 2024
46 checks passed
LogFilter() {}

void apply(Logits& logits) override {
OPENVINO_ASSERT(logits.is_vector_initialized());
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is true only if we have top_p or top_k set. Without them we don't need to sort results hence new vector is not initialized. So it will not work if only temperature is provided.
To make it work in both scenarios we need to change logits.m_vector[i].m_log_prob values if vector is initialized and
logits.m_data[i] if vector is not initialized.

github-merge-queue bot pushed a commit that referenced this pull request Sep 25, 2024
Fixed regression after
#886.
Turned on multinomial tests without ref comparison.
@ilya-lavrenov ilya-lavrenov added the category: sampling Sampling / Decoding algorithms label Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: sampling Sampling / Decoding algorithms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants