Repository navigation
fix(sentence-structure): align feature names, flooring, and rubric boundaries #404
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
fa11fc9
fix(sentence-structure): align rubric keys and preserve fractional fe…
slee430 e81747a
experiment(sentence-structure): compare raw flooring with explicit ru…
slee430 9b688ce
test(sentence-structure): guard percentage boundaries in classifier r…
slee430 d7aa30c
Merge branch 'main' into codex/fix-sentence-structure-features
slee430 82d5ee5
fix(sentence-structure): address review cleanup
slee430 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
8 changes: 4 additions & 4 deletions
8
evals/text-complexity/ela-reading/sentence-structure/rubric-grades-5-12.txt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
|
|
||
| **Slightly Complex:** A text is in the Slightly Complex bucket if it has at least 50% simple sentences. If it doesn't, the text is a higher level of complexity. If the % of simple sentences is >= 50% and the % of compound sentences is >= 20%, the text is Moderately Complex, otherwise, the text is Slightly Complex. Slightly Complex texts NEVER have advanced complex sentences — the presence of an advanced complex sentence always leads to a higher level of complexity than Slightly. | ||
| **For Moderately Complex:** These texts can take on any distribution of sentence types as long as there aren't more than 2 advanced complex sentences and as long as there aren't so many simple sentences that the text becomes Slightly Complex. That means Moderately Complex texts may have many simple sentences (although not so many that the text is Slightly Complex), compound sentences, and/or basic complex sentences. It's also possible for a moderately complex text to contain one or two advanced complex sentences, as long as there aren't more than 2. If there are more than 2, then the text is either Very or Exceedingly complex. | ||
| **Very Complex:** These texts contain 3 or more advanced complex sentences (unless the percentage of advanced complex sentences is >= 65)%, in which case the text becomes Exceedingly Complex). They may still contain many simple, compound, and basic complex sentences, but a text is not Very Complex unless there are 3 or more advanced complex sentences. | ||
| **Exceedingly Complex:** These texts have 65%+ of their sentences being advanced complex sentences. | ||
| **Slightly Complex:** A text is in the Slightly Complex bucket if it has >= 50% simple sentences. If it doesn't, the text is a higher level of complexity. If the % of simple sentences is >= 50% and the % of compound sentences is >= 20%, the text is Moderately Complex, otherwise, the text is Slightly Complex. Slightly Complex texts NEVER have advanced complex sentences — the presence of an advanced complex sentence always leads to a higher level of complexity than Slightly. | ||
| **For Moderately Complex:** These texts can take on any distribution of sentence types as long as there are < 3 advanced complex sentences and as long as there aren't so many simple sentences that the text becomes Slightly Complex. That means Moderately Complex texts may have many simple sentences (although not so many that the text is Slightly Complex), compound sentences, and/or basic complex sentences. It's also possible for a moderately complex text to contain one or two advanced complex sentences, as long as there are < 3. If there are >= 3, then the text is either Very or Exceedingly complex. | ||
|
aychi1 marked this conversation as resolved.
Resolved
|
||
| **Very Complex:** These texts contain >= 3 advanced complex sentences (unless the percentage of advanced complex sentences is >= 65)%, in which case the text becomes Exceedingly Complex). They may still contain many simple, compound, and basic complex sentences, but a text is not Very Complex unless there are >= 3 advanced complex sentences. | ||
| **Exceedingly Complex:** These texts have >= 65% of their sentences being advanced complex sentences. | ||
6 changes: 3 additions & 3 deletions
6
...valuators/contracts/_generated/text_complexity/ela_reading/sentence_structure/config.json
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@slee430 - To confirm, the code was already sending
avg_words_per_sentence, but the rubric was referencingavg_sentence_length. The model was inferring on this own, and silently resolving this bug?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.