Skip to content

fix: normalize default weights to sum to 1.0 instead of 1.15 - #134

Open
ayushi2577 wants to merge 9 commits into
0-Shimanshu:mainfrom
ayushi2577:fix/normalize-scorer-weights
Open

fix: normalize default weights to sum to 1.0 instead of 1.15#134
ayushi2577 wants to merge 9 commits into
0-Shimanshu:mainfrom
ayushi2577:fix/normalize-scorer-weights

Conversation

@ayushi2577

@ayushi2577 ayushi2577 commented May 25, 2026

Copy link
Copy Markdown
Contributor

fixes issue - #116

Problem

The default _weights in scorer.py summed to 1.15 instead of 1.0,
inflating scores near the flag (0.25) and throttle (0.55) thresholds.
The min(total, 1.0) clamp hid the issue for high scores but silently
caused overly aggressive verdicts for borderline requests.

Also, the ConfigSnapshot override path only replaced payload, behavior,
and identity weights — leaving context and ip_rep at inflated defaults.

Fix

  • Normalized _weights so they sum exactly to 1.0
  • Added normalization in the ConfigSnapshot override path

Fixes #128

@github-actions github-actions Bot added the enhancement New feature or request label May 25, 2026
Comment thread adiuvare/core/scorer.py
Comment thread adiuvare/core/scorer.py
Comment thread adiuvare/core/scorer.py
Comment thread tests/test_scorer.py Outdated
@Wahid7852

Copy link
Copy Markdown
Collaborator

Overall though, the bug and the reasoning behind the fix look legitimate but hey, can you please check these reviews out and just do a minor tweaks in your changes. The more tests and secure the code, the better it is, right?

@Wahid7852 Wahid7852 assigned Wahid7852 and ayushi2577 and unassigned Wahid7852 May 25, 2026
@Wahid7852

Copy link
Copy Markdown
Collaborator
image 🕵🏼 was it AI and your AI messed it up? lol

@ayushi-ag-25

Copy link
Copy Markdown

well i got confused in the issue numbers and for the test part i will do it soon and would be back with results

@ayushi2577

ayushi2577 commented May 26, 2026

Copy link
Copy Markdown
Contributor Author

Hey @Wahid7852 ,
I went through all your comments and made the changes:

  1. Added validation in the snap path for negative weights and zero sum — both raise ValueError now
  2. Added test_negative_weight_raises and test_all_zero_weights_raises as the safeguards you suggested
  3. Added test_identity_heavy_detection to confirm identity-heavy signals still work after the weight drop
  4. Switched assertions to pytest.approx(rel=1e-3) as you suggested
  5. Updated the command probe threshold since the score naturally changed after normalization — not a regression

for the testing part below are the screenshots
Screenshot 2026-05-26 201028
Screenshot 2026-05-26 210827

Also, honestly the zero-sum edge case was a good catch, I hadn't thought about what happens when someone passes all zeros through config, that could've been a silent bug for a while

@ayushi2577 ayushi2577 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@Wahid7852, i went through all your comments and basically i have added validation for negative and zero-sum weights (both raise ValueError now), added the three tests you suggested, switched assertions to pytest.approx, and verified identity-heavy detections still work fine after the weight drop. Should be good to go! i have also attached the screenshots of tests in my last comment on pr

@Wahid7852

Copy link
Copy Markdown
Collaborator

hey @ayushi2577 the validation and tests look good, before we merge, total_snap == 0 should be <= 0 to also catch edge cases where the sum is negative, and test_identity_heavy_detection uses score > 0.15 but the normalized identity weight is exactly 0.15 so that assertion would fail a correct result, needs to be tightened up.

@Wahid7852 Wahid7852 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

^

@ayushi2577

Copy link
Copy Markdown
Contributor Author

Fixed both
Changed == 0 to <= 0 to also catch negative sum edge cases, and tightened the identity test to pytest.approx(0.20, rel=1e-3) and pytest.approx(0.15, rel=1e-3) for the breakdown. Thanks for the sharp catch!

@ayushi2577 ayushi2577 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

you can review it now as I have done all requested changes

Comment thread adiuvare/core/scorer.py Outdated
@ayushi2577

Copy link
Copy Markdown
Contributor Author

Resaved as plain UTF-8

@0-Shimanshu
0-Shimanshu self-requested a review May 30, 2026 06:08
@Wahid7852 Wahid7852 added quality:clean GSSoC quality: clean implementation type:testing GSSoC type bonus: testing gssoc:approved GSSoC validation: approved labels May 30, 2026
@Wahid7852 Wahid7852 added level:intermediate GSSoC difficulty: intermediate type:bug GSSoC type bonus: bug fix labels May 30, 2026

@Wahid7852 Wahid7852 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

final changes. promise.

Comment thread adiuvare/core/scorer.py
Comment thread adiuvare/core/scorer.py
Comment thread adiuvare/core/scorer.py
@ayushi2577

Copy link
Copy Markdown
Contributor Author

Done, I have updated the error message to say 'snap weights', added the assert for the default path, and added a comment explaining why context and ip_rep aren't validated.

@Wahid7852 Wahid7852 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@0-Shimanshu

Copy link
Copy Markdown
Owner

@0-Shimanshu LGTM

i will do it after i check one thing
because i closed one other pr for this same issue

@ayushi2577

Copy link
Copy Markdown
Contributor Author

@0-Shimanshu I request to review and merge it

@ayushi2577

Copy link
Copy Markdown
Contributor Author

@Wahid7852 and @0-Shimanshu
I expect there are no other changes needed can u review and merge it?

@0-Shimanshu

Copy link
Copy Markdown
Owner

@ayushi2577 did you check the normalising function ?
i think i had a normalising function to make sum to 1 even if it exceeds/.

@ayushi2577

Copy link
Copy Markdown
Contributor Author

Yeah I checked ,
the original code only had normalization in the snap override path (if total_w > 0), but the default _weights dict itself was never normalized and summed to 1.15. My fix normalizes _weights at module load time so the default path is also always exactly 1.0.

@Wahid7852

Copy link
Copy Markdown
Collaborator

@0-Shimanshu merge?

@0-Shimanshu

Copy link
Copy Markdown
Owner

@0-Shimanshu merge?

kinda confused about this cause decreasing value is not the right way to go i think here
because there possibilty user introduce new signal so new weight then same problem then also will come i think

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request gssoc:approved GSSoC validation: approved level:intermediate GSSoC difficulty: intermediate quality:clean GSSoC quality: clean implementation type:bug GSSoC type bonus: bug fix type:testing GSSoC type bonus: testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature]: Improve Windows setup instructions in README

4 participants