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

Add utility functions for text processing and visualization #17

Merged
merged 18 commits into from
Feb 13, 2024
Merged

Conversation

jaidhyani
Copy link
Collaborator

This partially addresses #9 (jettjaniak/tinyevals#3)

Including a notebook to demonstrate usage

@jaidhyani
Copy link
Collaborator Author

I've added an adapted version of https://github.com/jettjaniak/tinyevals/pull/25 by @siwei-li in compare_models.py

@jettjaniak
Copy link
Contributor

let's move it to delphi/evals/vis.py

src/delphi/vis/vis.py Outdated Show resolved Hide resolved
src/delphi/vis/vis.py Outdated Show resolved Hide resolved
src/delphi/vis/utils.py Outdated Show resolved Hide resolved
src/delphi/vis/utils.py Outdated Show resolved Hide resolved
@jettjaniak
Copy link
Contributor

As mentioned in the original PR on old repo, I don't think it's worth optimizing for performance in the visualization code. It feel it could be much simpler if we dropped the vectorization

@jettjaniak jettjaniak mentioned this pull request Feb 3, 2024
@jettjaniak
Copy link
Contributor

jettjaniak commented Feb 9, 2024 via email

@jettjaniak
Copy link
Contributor

I think you can use sth like pyproject.toml to configure isort in all 3 instances (pre-commit, CI, vscode), but I'm fine with either solution

Copy link
Contributor

@jettjaniak jettjaniak left a comment

Choose a reason for hiding this comment

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

thanks for making all of these changes!
I suggested a bunch of simplifications

src/delphi/eval/compare_models.py Outdated Show resolved Hide resolved
src/delphi/eval/compare_models.py Outdated Show resolved Hide resolved
src/delphi/eval/compare_models.py Outdated Show resolved Hide resolved
src/delphi/eval/utils.py Outdated Show resolved Hide resolved
src/delphi/eval/utils.py Outdated Show resolved Hide resolved
src/delphi/eval/vis.py Outdated Show resolved Hide resolved
tests/test_vis.py Outdated Show resolved Hide resolved
tests/test_vis.py Outdated Show resolved Hide resolved
src/delphi/eval/utils.py Outdated Show resolved Hide resolved
@jaidhyani
Copy link
Collaborator Author

Everything should be addressed.

@jaidhyani jaidhyani requested a review from jettjaniak February 11, 2024 06:42
@jaidhyani jaidhyani linked an issue Feb 11, 2024 that may be closed by this pull request
Copy link
Contributor

@jettjaniak jettjaniak left a comment

Choose a reason for hiding this comment

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

Great, thank you! Commented on one bug, please make sure to correct it before merging.

Comment on lines 70 to 54
next_probs_a, probs_a = get_correct_and_all_probs(model_a, sample_tok)
next_probs_b, probs_b = get_correct_and_all_probs(model_b, sample_tok)
probs_a, next_probs_a = get_all_and_next_logprobs_single(model_a, sample_tok)
probs_b, next_probs_b = get_all_and_next_logprobs_single(model_b, sample_tok)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this probs or logprobs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

! Good catch!

Comment on lines +57 to +64
def get_next_and_top_k_probs(
model: PreTrainedModel, input_ids: Int[torch.Tensor, "seq"], k: int = 3
) -> tuple[Float[torch.Tensor, "shorter_seq"], torch.return_types.topk,]:
all_logprobs, next_logprobs = get_all_and_next_logprobs_single(model, input_ids)
all_probs = torch.exp(all_logprobs)
next_probs = torch.exp(next_logprobs)
top_k = torch.topk(all_probs, k, dim=-1)
return next_probs, top_k
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you using this function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the demo notebook

@jaidhyani jaidhyani merged commit 4ed8b19 into main Feb 13, 2024
1 check passed
@jaidhyani jaidhyani deleted the issue9 branch March 20, 2024 00:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

prompt vis
3 participants