-
Notifications
You must be signed in to change notification settings - Fork 51
feat: add single token feature detection and logging #91
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
feat: add single token feature detection and logging #91
Conversation
- Add is_single_token field to LatentRecord - Implement single token detection in constructors - Update scorer and analysis to track single token metrics - Log single token ratio in verbose mode Closes EleutherAI#87
for more information, see https://pre-commit.ci
|
If you are willing to give it another shot, I had already started in #89. I think it should be done after caching and not when you are building the feature examples (although I think having and option to skip single token latents is nice). Maybe we can have both of them - my method as a diagnostics tool that is used when verbose it true, and your suggestion as maybe an argument (skip_single_token ?) that can be used to not explain single token latents? @luciaquirke, @norabelrose maybe you have ideas? |
| max_examples = constructor_cfg.max_examples | ||
| min_examples = constructor_cfg.min_examples | ||
|
|
||
| token_windows, act_windows = pool_max_activation_windows( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this call has been duplicated by mistake. I don't believe this code will run. I have added documentation on how to run the tests to the README.md.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@meghana-0211 try pytest . and python -m delphi.tests.e2e.
| # For each example, check if activation is concentrated in a single position | ||
| max_activations = activations.max(dim=1).values | ||
| top_k = int(len(max_activations) * quantile_threshold) | ||
| top_indices = max_activations.topk(top_k).indices |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that the activations are already in top k order (tbh they should probably be renamed to ordered_example_acts or similar to reflect this, starting in _top_k_pools). So we should be able to directly slice the first len(activations) * quantile_threshold activations.
| # Calculate ratio of single token activations | ||
| single_token_ratio = (significant_activations == 1).float().mean().item() | ||
|
|
||
| return single_token_ratio >= activation_ratio_threshold |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it doesn't look like this method actually checks whether the activations are single token 🤔 maybe I'm just confused but I can't see it
|
Looks like there may have been a miscommunication - single token feature is supposed to mean that the feature fires on the same token every time, not that it fires on one token within each sequence. This code also won't run as is. That said, the code in result_analysis looks great and could be pulled verbatim into a future PR for this issue. @SrGonao I'm curious what the issue is with doing this type of non-LLM calling data processing in the constructor, could you say a bit more about your reasoning? |
|
I think if we want to get statistics as a diagnostics tool it should be done after caching. |
|
Yeah I think we probably just want statistics and not skipping single token explanation to keep things simple for now. I trust that we shouldn't collect statistics before caching is complete, am curious as to the reasoning. |
|
Sorry, by after caching I meant right after. I think slowing down the constructor leads to underutilization of the GPUs and so in my mind I would like to keep as much "work" that can be done beforehand out of the constructor. Maybe it doesn't make a difference though, I just thought it to be more natural. |
|
Closing due to inactivity |
Single Token Feature Detection
Adds functionality to detect and log features that primarily activate on single tokens, as requested in #87.
Changes
is_single_tokenfield toLatentRecordconstructors.pyImplementation Details
Closes #87