Skip to content

Make float_equiv's warning and docstring state the value it returns - #4356

Open
buzzedaldrinx86-creator wants to merge 2 commits into
stanford-crfm:mainfrom
buzzedaldrinx86-creator:fix-float-equiv-warning-text
Open

buzzedaldrinx86-creator wants to merge 2 commits into
stanford-crfm:mainfrom
buzzedaldrinx86-creator:fix-float-equiv-warning-text

Conversation

@buzzedaldrinx86-creator

Copy link
Copy Markdown

Addresses item 1 of #4340. No scoring behaviour changes.

conv_fin_qa_calc_metrics.float_equiv returns 0.0 when either value cannot be parsed as a float, but its runtime warning says float_equiv returning 1.0 because both values are non-floats, and the docstring says the same. Anyone reading the logs to understand a score sees "returning 1.0" while the metric receives 0.0, and "both" while the guard is ss1 is None or ss2 is None.

This PR takes the smallest option from the issue: the warning and the docstring now describe what the code does.

  • Warning: float_equiv returning 0.0 because at least one value is a non-float
  • Docstring: "If either value cannot be parsed as a float, then it will always return 0.0, regardless of whether the strings match."
  • Adds test_conv_fin_qa_calc_metrics.py, which pins the existing return values (equal floats 1.0, different floats 0.0, any non-float 0.0), so a later change to this path is a visible decision.

I did not change the returned value: 1.0 may have been the original intent, but switching it would alter existing ConvFinQA scores, which is a maintainer decision. The except Exception: return float(str1 == str2) fallback is also left alone. Happy to follow up on either if you want a different direction.

Checked

The function bodies from main (63754d05) and from this branch were executed with HELM's imports stubbed: every return value is identical before and after, and the warning now states the returned value (before: stated 1.0, returned 0.0). black --check (line length 120) and flake8 pass on the two files. I have not run the full HELM test suite locally.

🤖 Generated with Claude Code

Addresses item 1 of stanford-crfm#4340. No scoring behaviour changes.

float_equiv returns 0.0 when either value cannot be parsed as a float,
but its runtime warning and docstring both said it returns 1.0 when
both values are non-floats. The warning and docstring now describe
what the code does. Adds a test that pins the existing return values.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…e docstring

The test added earlier pins return values, which this PR does not change,
so it passed on main. The new test patches hwarn and asserts that the
warning names the value actually returned; it fails on main and passes here.

The docstring said the function "always" returns 0.0 for non-floats.
Non-str inputs such as None reach the outer except and return 1.0, so the
sentence now describes the string case only.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@buzzedaldrinx86-creator

Copy link
Copy Markdown
Author

Pushed a follow-up after a second look at my own PR. The test I first added only pinned return values, which this PR doesn't change, so it passed on main and didn't guard the fix. The new test patches hwarn and asserts the warning names the value actually returned: it fails on main ("returning 1.0" while 0.0 is returned) and passes on this branch. I also dropped "always" from the docstring line, since non-str inputs like None fall through to the outer except and return 1.0. No scoring behaviour changes.

@buzzedaldrinx86-creator

Copy link
Copy Markdown
Author

Gentle ping on this one when there's time, @yifanmai. It only changes float_equiv's warning text and docstring (no scoring behaviour changes). The added test fails on main and passes on this branch. I understand HELM is in maintenance mode, so if this is out of scope I'm happy for it to be closed, or to trim it further.

This branch has not been deployed

No deployments
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.

1 participant