fix(sentence-structure): align feature names, flooring, and rubric boundaries by slee430 · Pull Request #404 · learning-commons-org/evaluators

slee430 · 2026-10-28T23:36:17Z

Summary

The grade 3–4 rubrics use avg_sentence_length, but the model receives avg_words_per_sentence. Python and TypeScript also convert numeric features differently.

This PR fixes the feature-name mismatch and applies the same floor approach in both SDKs and the notebook.

Changes

Replace all eight outdated feature-name references in the grade 3–4 rubrics.
Floor raw values without rounding to one decimal first.
Use < and >= in the grade 3–4 rubrics. Write ranges as >= a and < b.
Rewrite the grades 5–12 conditions using equivalent inequalities.
Update rubric hashes and bundled Python files.
Add boundary tests to the existing test files.

For example, 44.96% becomes 44 and stays below 45%. Changing > 60% to >= 60% also means exactly 60% now meets that criterion.

Validation

All 6 repository checks passed.
Python make verify passed: 1,755 tests passed; 108 skipped.
TypeScript lint and typecheck passed; all 33 sentence-structure tests passed after the review cleanup.
Review cleanup left the full model requests unchanged for all 407 saved cases; no new model calls were needed.
Added 12 mocked boundary cases per SDK. They check the numbers and rubric instructions sent to the model.
Python, TypeScript, and the notebook produced identical feature JSON for all 407 comparison cases.

Classification comparison

Each version was run three times on 402 grade 3–4 rows using Sentence Score as the reference label. The runs used gpt-4o-2024-08-06, temperature 0, and the same saved grammatical analyses.

Version	Mean accuracy
Original Python baseline	57.13%
Initial decimal-preserving PR	57.38%
Floor + updated boundaries	58.13%

Compared with the initial PR, five grade 3 cases and one grade 4 case became incorrect in all three runs. The five repository fixture/integration cases had unchanged labels across versions and runs.

The overall gain does not establish a general accuracy improvement. These runs did not use held-out data or repeat the grammatical-analysis step.

Known limitations and follow-up

A floating-point issue remains in both SDKs and the notebook. For example, 29 / 100 * 100 can produce 28.999999999999996, which floors to 28 instead of 29. This issue was not found in the 407 comparison cases.

Flooring also removes fractional signals: an average of 0.4 subordinate clauses per sentence becomes 0. The current rubrics do not set explicit thresholds for this feature, so this behavior is kept for now as a known limitation.

Further review and revision are needed for the calculation issue, the usefulness of fractional features, and the persistent classification regressions. Validation on held-out data remains a follow-up.

…atures

codecov · 2026-10-28T23:37:40Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

adnanrhussain

@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.

adnanrhussain · 2026-10-29T03:59:34Z

    * **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).


@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?

@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.

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.

Hi @adnanrhussain @slee430 -- I've looked at the code and also viewed Seung Yeon's tests. Just to confirm my understanding: Adnan's approach means that the issue Seung Yeon was originally aiming to fix (i.e. some variables such as percentages get truncated) will not be resolved, to keep things consistent with the SDK. Is that understanding correct?

On my end, I'm fine with this for now and I think it's something we should flag for fixing later on.

…bric boundaries

…equests

adnanrhussain

Conditional approval, requested changes pls. TY

adnanrhussain · 2026-11-05T22:39:16Z

-def features_to_json(features: Mapping[str, Any]) -> str:
+def features_to_json(
+    features: Mapping[str, Any], *, cast_to_int: bool = True, floor_values: bool = False
+) -> str:
    """:data:`FEATURE_COLS` as the JSON object bound to ``{sentence_features}``.

-    Every value is a whole number: the rubric's thresholds are written in whole percentages
-    and averages, and the fractional place the model would otherwise read invites precision
-    it cannot support. A feature the analysis did not produce is sent as ``null`` rather
-    than zero, so a missing count reads as missing rather than as "none found".
+    ``floor_values=True`` floors the supplied values without preliminary rounding and
+    takes precedence over ``cast_to_int``. Supply unrounded features for this mode.
+    Otherwise, ``cast_to_int=False`` retains one decimal place. Defaults preserve the
+    legacy serializer API.
+    A feature the analysis did not produce is sent as ``null`` rather than zero.
    """
-    payload: dict[str, int | None] = {}
+    payload: dict[str, int | float | None] = {}
    for name in FEATURE_COLS:
        value = features.get(name)
        if isinstance(value, bool) or not isinstance(value, (int, float)):
            payload[name] = None
        elif isinstance(value, int):
            payload[name] = value
+        elif floor_values:
+            payload[name] = math.floor(value)
        else:
-            # Truncated, not rounded: this is the notebook's arithmetic, and the fixtures
-            # and thresholds this evaluator was validated against were produced by it.
-            payload[name] = int(value)
+            payload[name] = int(value) if cast_to_int else round(value, PRECISION)
    return json.dumps(payload, indent=2)


P0 - Can we pls remove the cast_to_int since it was introduced in this PR but not used now

Removed cast_to_int from the Python helper in 82d5ee5. The default still truncates to integers, and the evaluator still uses floor_values=True. I also checked that the full model requests are unchanged for all 407 saved cases.

adnanrhussain · 2026-11-05T22:39:56Z

+@pytest.mark.parametrize(
+    ("name", "value", "expected"),
+    [
+        ("avg_transitions_per_sentence", 0.20, 0.2),
+        ("avg_subordinates_per_sentence", 0.43, 0.4),
+        ("appositive_phrase_density", 0.52, 0.5),
+        ("participle_phrase_density", 0.29, 0.3),
+        ("percent_simple_sentences", 60.5, 60.5),
+    ],
+)
+def test_fractional_signals_survive_serialization(name: str, value: float, expected: float) -> None:
+    payload = json.loads(features_to_json({name: value}, cast_to_int=False))
+    assert payload[name] == expected
+    assert payload["num_sentences"] is None


Can be removed, since we no longer use the rounding to implementation

Removed the test for cast_to_int=False in 82d5ee5. The flooring and boundary tests are still in place.

adnanrhussain · 2026-11-05T22:41:00Z

@@ -83,7 +83,7 @@ export class SentenceStructureEvaluator extends defineMultiStepEvaluator<
    calculateReadabilityMetrics: (text) => formatGroundTruthCounts(String(text)),
    // Cast to int and single-space, matching the Python implementation's JSON.


Now stale-comment. please update

Updated the comment in 82d5ee5. It now says that we floor unrounded values before serialization, matching the Python evaluator.

adnanrhussain · 2026-11-05T22:44:43Z

    // Cast to int and single-space, matching the Python implementation's JSON.
    addEngineeredFeatures: (analysis) =>
-      featuresToJSON(addEngineeredFeatures(analysis as SentenceAnalysis), 1, true),
+      featuresToJSON(addEngineeredFeatures(analysis as SentenceAnalysis), 1, true, true),


If the last arg floorValues is true, it would short-circuit and ignore the decimals 1 and castToInt true.

We should refactor this for better readability, but its outside the scope of this PR. Can you please add a TODO over to update with calling with objects?

Added the TODO in 82d5ee5 to use an options object. It also notes that floorValues=true ignores decimals and castToInt. The refactor is left for a separate change.

adnanrhussain · 2026-11-05T22:45:49Z

-    # truncate-versus-round question it belongs with.
-    features.update({name: round(value, PRECISION) for name, value in derived.items()})
+    # Preserve the notebook's ties-to-even rounding; cross-SDK rounding alignment
+    # is tracked separately in DSCR-2223.


Stale comment

Updated the comment in 82d5ee5. It now explains that default callers keep one-decimal rounding, while the sentence-structure evaluator uses round_values=False before flooring.

aychi1

LGTM. One minor comment (edge case) on a prompt.

fix(sentence-structure): align rubric keys and preserve fractional fe…

fa11fc9

…atures

slee430 requested a review from a team as a code owner October 28, 2026 23:36

slee430 requested review from adnanrhussain, aychi1, czi-fsisenda and michael-berger-czi October 29, 2026 00:21

adnanrhussain assigned Copilot Oct 29, 2026

adnanrhussain reviewed Oct 29, 2026

View reviewed changes

slee430 added 2 commits October 30, 2026 15:11

experiment(sentence-structure): compare raw flooring with explicit ru…

e81747a

…bric boundaries

test(sentence-structure): guard percentage boundaries in classifier r…

9b688ce

…equests

slee430 changed the title ~~fix(sentence-structure): align rubric keys and preserve fractional features~~ fix(sentence-structure): align feature names, flooring, and rubric boundaries Nov 5, 2026

Merge branch 'main' into codex/fix-sentence-structure-features

d7aa30c

adnanrhussain approved these changes Nov 5, 2026

View reviewed changes

fix(sentence-structure): address review cleanup

82d5ee5

aychi1 approved these changes Nov 6, 2026

View reviewed changes

Comment thread evals/text-complexity/ela-reading/sentence-structure/rubric-grades-5-12.txt Resolved

slee430 merged commit e1b5dfb into main Nov 6, 2026
24 checks passed

slee430 deleted the codex/fix-sentence-structure-features branch November 6, 2026 21:07

This was referenced Nov 6, 2026

chore(main): release evals-prompts 1.10.1 #423

Open

chore(main): release sdks-typescript 1.3.1 #424

Open

chore(main): release sdks-python 1.2.1 #425

Open

		@@ -83,7 +83,7 @@ export class SentenceStructureEvaluator extends defineMultiStepEvaluator<
		calculateReadabilityMetrics: (text) => formatGroundTruthCounts(String(text)),
		// Cast to int and single-space, matching the Python implementation's JSON.

Conversation

slee430 commented Oct 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Validation

Classification comparison

Known limitations and follow-up

Uh oh!

codecov Bot commented Oct 28, 2026

Codecov Report

Uh oh!

adnanrhussain left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adnanrhussain left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aychi1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

slee430 commented Oct 28, 2026 •

edited

Loading