Skip to content

dedupe code in all.py and single.py#76

Open
anthonyduong9 wants to merge 1 commit intomainfrom
dedupe-code-in-all-and-single
Open

dedupe code in all.py and single.py#76
anthonyduong9 wants to merge 1 commit intomainfrom
dedupe-code-in-all-and-single

Conversation

@anthonyduong9
Copy link
Contributor

@anthonyduong9 anthonyduong9 commented Apr 24, 2025

Problem

  • all.py and single.py both contain functions _get_safe_dtype() and _safe_cast()
  • all.py and single.py duplicate get_layer_num_from_sae_id()
  • A large block of code to calculate DFA values has been practically duplicated in all.py and single.py, and so it seems unlikely we'll want to change it in two places.

Fix

  • Moving one pair of functions _get_safe_dtype() and _safe_cast() to shared.py and deleting the other pair.
  • Moving get_layer_num_from_sae_id() to shared.py.
  • Extracting the large block of code to calculate_per_source_dfa().

Testing

@codecov
Copy link

codecov bot commented Apr 24, 2025

Codecov Report

❌ Patch coverage is 80.00000% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 8.49%. Comparing base (e8ddd08) to head (9c7f58b).

Files with missing lines Patch % Lines
apps/inference/neuronpedia_inference/shared.py 76.66% 5 Missing and 2 partials ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##            main     #76      +/-   ##
========================================
+ Coverage   7.86%   8.49%   +0.62%     
========================================
  Files        124     124              
  Lines      17470   17448      -22     
  Branches     381     379       -2     
========================================
+ Hits        1374    1482     +108     
+ Misses     16085   15950     -135     
- Partials      11      16       +5     
Flag Coverage Δ
autointerp 100.00% <ø> (ø)
inference 45.02% <80.00%> (+8.24%) ⬆️
webapp 5.12% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@anthonyduong9 anthonyduong9 marked this pull request as ready for review May 3, 2025 08:47
@anthonyduong9 anthonyduong9 force-pushed the dedupe-code-in-all-and-single branch from 3b51380 to 2dfc4d2 Compare May 7, 2025 01:28
@hijohnnylin
Copy link
Owner

@anthonyduong9 thanks for the update, sorry for slowness on getting around to it!

we need test(s) that the before/after results are the same before merging this - can you write them, or let me know what info you'd need to write them? if you don't have time for it lmk - i can work on it.

@anthonyduong9 anthonyduong9 marked this pull request as draft July 16, 2025 22:59
@anthonyduong9 anthonyduong9 force-pushed the dedupe-code-in-all-and-single branch from 989a1f3 to a3c2ed0 Compare July 21, 2025 04:11
@anthonyduong9 anthonyduong9 force-pushed the dedupe-code-in-all-and-single branch from a3c2ed0 to 9c7f58b Compare August 14, 2025 00:24
@anthonyduong9
Copy link
Contributor Author

@hijohnnylin I've added tests for calculate_dfa() and ActivationProcessor.process_activations(), since if you start from the functions where we extracted calculate_per_source_dfa() and go to where they're called, and so on, those're the public functions you end up at. They pass on main (save for one that references calculate_per_source_dfa(), since it doesn't exist), as well as this branch. The tests for calculate_dfa() largely match this in SAEDashboard, so assuming it's correct, we can have more confidence.

@anthonyduong9 anthonyduong9 marked this pull request as ready for review August 14, 2025 00:44
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.

2 participants