Repository navigation
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
adnanrhussain
left a comment
There was a problem hiding this comment.
@slee430 - After reviewing the code, I think there are more inconsistencies between the SDKs. For example, I think the JSON.stringify to convert the features and send to the model, would not send a 10.0 and instead send a 10. This would be different behavior than the Python impl.
Since most of the code already truncates today, perhaps we could try the alternative path of -
- flooring the raw values to whole numbers
- rubric thresholds to use only < and >=, with ranges written as "≥ a and < b"
- the floor has to apply to the unrounded ratio
This is a subtle change, meeting the original intent of the rubric, but changing its current behavior, so will require re-running the tests you validated for this PR, but now with this change.
| * **Statistical Guidelines:** The text is likely "Slightly Complex" if it meets at least TWO of the following criteria: | ||
| * **Sentence Type:** Primarily simple sentences. (`percent_simple_sentences` is typically > 60%). | ||
| * **Sentence Length:** Short sentences. (`avg_sentence_length` is typically < 12 words). | ||
| * **Sentence Length:** Short sentences. (`avg_words_per_sentence` is typically < 12 words). |
There was a problem hiding this comment.
@slee430 - To confirm, the code was already sending avg_words_per_sentence, but the rubric was referencing avg_sentence_length. The model was inferring on this own, and silently resolving this bug?
There was a problem hiding this comment.
@adnanrhussain Yes, the code was already sending avg_words_per_sentence, but the grade 3 and 4 rubrics referenced avg_sentence_length. The model may have inferred the connection, but we cannot confirm that it did so consistently. This fix makes the reference explicit.
There was a problem hiding this comment.
Regarding your alternative approach, I’ll keep the current implementation for comparison and test your proposal in a separate branch across Python, TypeScript, and the notebook. I’ll keep the feature-name fix in both versions and make the rubric boundary changes explicit.
Then I’ll rerun the tests and compare the baseline, the current PR, and your proposal using the same extracted features. I’ll check the grade 4 results closely, since accuracy decreased there in the previous comparison. We can review the results before deciding which approach to use in this PR.
Summary
The grade 3 and 4 sentence-structure rubrics reference
avg_sentence_length, but the classifier receivesavg_words_per_sentence. The notebook and Python SDK also truncate fractional features to integers before classification; the TypeScript SDK rounds them to whole numbers. For example, four subordinate clauses across ten sentences becomes0instead of0.4per sentence. Details are described in this doc.This PR:
cast_to_intoption to the Python serializer, keeping its existing default for compatibility and explicitly disabling integer conversion in this evaluator.The precision change applies to all serialized numeric features, including percentages and sentence-length averages. It retains one-decimal rounding; Python/TypeScript tie-breaking differences remain outside this change.
Validation
make verify: passed, including 1,705 tests; 107 skipped.Classification impact — review before release
A separate 402-row grade 3–4 evaluation used human Sentence Score labels,
gpt-4o-2024-08-06, temperature 0, and shared stage-one counts across configurations. The baseline reproduces the original Python SDK behavior, not historical saved predictions.With both fixes, 37 labels changed: 14 became correct, 16 became incorrect, and 7 remained incorrect. Grade 3 gained 5 correct cases; grade 4 lost 7. The overall paired accuracy difference was −0.50 percentage points (95% passage-clustered bootstrap interval: −3.21 to +2.22). Two additional baseline/fixed repeats on the changed cases and 40 unchanged controls found 9 of the initial regressions persisted across all three runs.
These are confirmed input-contract/precision defects, but the combined change is not established as performance-neutral. Please review whether the rubric rename and precision change should ship together or separately, particularly given the grade 4 regressions. Additional SDK changes and tests on this PR branch are welcome.
All relevant data and codes are in this google drive folder.