Skip to content

Modify sig fig feedback priority order - #818

Open
axlewin wants to merge 8 commits into
mainfrom
hotfix/sig-fig-feedback-order
Open

Modify sig fig feedback priority order#818
axlewin wants to merge 8 commits into
mainfrom
hotfix/sig-fig-feedback-order

Conversation

@axlewin

@axlewin axlewin commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

The feedback priority order should now be:

  1. Too many significant figures, but otherwise correct
  2. Default feedback, if present
  3. Too few significant figures, regardless of correctness

This makes numeric questions consistent with coordinate questions, where this was already the feedback order.

Note that we now always do sig fig checking (unless diregardSignificantFigurers is set) and overwrite the sig_figs_too_few feedback with the default feedback if necessary, instead of short-circuiting with the default feedback before the sig figs check. IMO this makes the code easier to follow since we're now only calling useDefaultFeedbackIfNecessary in one place, but we could add the short-circuit option back in between the two types of sig fig check.

Also moves the hardcoded sig_figs feedback tags to constants.

axlewin added 4 commits August 5, 2026 13:56
If the user's answer is correct but has too many significant figures, specific feedback about this is more useful than the default feedback.

The "too few s.f., regardless of correctness" feedback should still be lower priority than the default feedback, since we have no way of knowing whether the answer was rounded too much or is just wrong.
This isn't an issue for normal use since tags cannot be null in a real question, but it breaks some tests where tags aren't manually initialised.
@codecov

codecov Bot commented Aug 6, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.50000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 41.01%. Comparing base (b4f8f70) to head (9a267dd).
⚠️ Report is 17 commits behind head on main.

Files with missing lines Patch % Lines
...c/cam/cl/dtg/isaac/quiz/IsaacNumericValidator.java 83.33% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #818      +/-   ##
==========================================
+ Coverage   40.97%   41.01%   +0.03%     
==========================================
  Files         550      550              
  Lines       23555    23583      +28     
  Branches     2837     2845       +8     
==========================================
+ Hits         9652     9672      +20     
- Misses      13032    13039       +7     
- Partials      871      872       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@axlewin
axlewin marked this pull request as ready for review August 6, 2026 15:18

// If incorrect and we have not used the default validation response then go ahead and return it
// - this provides more helpful feedback than sig figs errors.
if (!bestResponse.isCorrect() && bestResponse.getExplanation() != null

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

By removing this, we also override answer-specific feedback with sig. figs. For an example, see https://staging.isaacscience.org/questions/ft_mechanics_core_7, Part D, and enter 0.4, None as the answer.

If we have exactly matched a known incorrect choice that also has the wrong number of sig figs, the custom feedback for that choice should take precedence over generic sig figs feedback.
This test needs to check that "too many sig figs" takes precedence over the correct explanation.
@axlewin

axlewin commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

FWIW, even if this gets approved before next week's release, I'd advocate not merging this until the release after; I don't think that modifying this behaviour mid-STEM SMART residential when the content teams won't be available to test is sensible.

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.

2 participants