Skip to content

Bring coord sig fig feedback in line with numeric questions #761

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.

Sign up for GitHub

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

Open
axlewin wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Bring coord sig fig feedback in line with numeric questions #761

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
884cfa1
Prevent coord sig figs defaulting to 2 when they should be ignored
axlewin Feb 25, 2026
67114d2
Bring coord sig fig feedback in line with numeric questions
axlewin Feb 27, 2026
12a8713
Fix subset sig figs logic
axlewin Feb 27, 2026
276825c
Prevent sig fig feedback on incorrect values
axlewin Feb 27, 2026
62f0e27
Show sig fig feedback for allowSubsetMatch choices
axlewin Feb 27, 2026
33ddd13
Simplify validator logic
axlewin Feb 27, 2026
fe5d039
Remove redundant null check
axlewin Feb 27, 2026
97ec917
Restore default feedback for correct choices
axlewin Feb 27, 2026
e732613
Add unit tests for more coordinate feedback cases
axlewin Mar 3, 2026
f110f00
Simplify test
axlewin Mar 3, 2026
209548b
Add variable for reused test coord item
axlewin Mar 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 52 additions & 11 deletions src/main/java/uk/ac/cam/cl/dtg/isaac/quiz/IsaacCoordinateValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@ public final QuestionValidationResponse validateQuestionResponse(final Question
IsaacCoordinateQuestion coordinateQuestion = (IsaacCoordinateQuestion) question;
CoordinateChoice submittedChoice = (CoordinateChoice) answer;

Integer sigFigsMin = coordinateQuestion.getSignificantFiguresMin();
Integer sigFigsMax = coordinateQuestion.getSignificantFiguresMax();

// STEP 0: Is it even possible to answer this question?

if (null == coordinateQuestion.getChoices() || coordinateQuestion.getChoices().isEmpty()) {
Expand Down Expand Up @@ -127,10 +130,6 @@ public final QuestionValidationResponse validateQuestionResponse(final Question
}
List<CoordinateItem> choiceItems = coordinateChoice.getItems().stream().map(i -> (CoordinateItem) i).collect(Collectors.toList());

// Get significant figures to validate with
Integer sigFigsMin = coordinateQuestion.getSignificantFiguresMin();
Integer sigFigsMax = coordinateQuestion.getSignificantFiguresMax();

// Check that the items in the submitted answer match the items in the choice numerically

boolean allItemsMatch = false;
Expand All @@ -151,10 +150,10 @@ public final QuestionValidationResponse validateQuestionResponse(final Question
CoordinateItem choiceItem = choiceItems.get(coordIndex);
CoordinateItem submittedItem = submittedItems.get(coordIndex);
// Check that the submitted item matches the choice item
if (!coordinateItemsMatch(submittedItem, choiceItem, sigFigsMin, sigFigsMax)) {
if (!coordinateItemsMatch(submittedItem, choiceItem, sigFigsMin, sigFigsMax, false)) {
allItemsMatch = false;
// Check if this is just a significant figures mismatch
if (!coordinateItemsMatch(submittedItem, choiceItem, null, null)) {
if (!coordinateItemsMatch(submittedItem, choiceItem, sigFigsMin, sigFigsMax, true)) {
allItemsMatchWithoutSigFigs = false;
// Exit early on mismatch:
break;
Expand All @@ -169,7 +168,8 @@ public final QuestionValidationResponse validateQuestionResponse(final Question
break;
} else if (allItemsMatchWithoutSigFigs) {
feedback = new Content(DEFAULT_VALIDATION_RESPONSE);
feedback.setTags(new HashSet<>(ImmutableList.of("sig_figs")));
// Too many sig figs, or too few sig figs but the choice has trailing zeros; otherwise correct
feedback.setTags(new HashSet<>(ImmutableList.of("sig_figs", "sig_figs_too_many")));
break;
}

Expand All @@ -179,23 +179,37 @@ public final QuestionValidationResponse validateQuestionResponse(final Question
// For correct choices, check if the submitted items are a proper subset of the choice
if (coordinateChoice.isCorrect() && (choiceItems.size() > submittedItems.size())) {
boolean allSubmittedItemsInChoiceItems = true;
boolean allItemsInChoiceWithoutSigFigs = true;
for (CoordinateItem submittedItem : submittedItems) {
boolean submittedItemInChoiceItem = false;
boolean itemInChoiceWithoutSigFigs = false;
for (CoordinateItem choiceItem : choiceItems) {
if (coordinateItemsMatch(submittedItem, choiceItem, sigFigsMin, sigFigsMax)) {
if (coordinateItemsMatch(submittedItem, choiceItem, sigFigsMin, sigFigsMax,
false)) {
submittedItemInChoiceItem = true;
break;
} else if (coordinateItemsMatch(submittedItem, choiceItem, sigFigsMin, sigFigsMax,
true)) {
itemInChoiceWithoutSigFigs = true;
}
}
if (!submittedItemInChoiceItem) {
allSubmittedItemsInChoiceItems = false;
break;
// Check if this is just a significant figures mismatch
if (!itemInChoiceWithoutSigFigs) {
allItemsInChoiceWithoutSigFigs = false;
break;
}
}
}
if (allSubmittedItemsInChoiceItems) {
feedback = new Content((submittedItems.size() == 1 ? "This is" : "These are")
+ " correct, but can you find more?");
break;
} else if (allItemsInChoiceWithoutSigFigs) {
feedback = new Content(DEFAULT_VALIDATION_RESPONSE);
feedback.setTags(new HashSet<>(ImmutableList.of("sig_figs", "sig_figs_too_many")));
break;
}
}

Expand All @@ -204,23 +218,35 @@ public final QuestionValidationResponse validateQuestionResponse(final Question
boolean allowSubsetMatch = (null != coordinateChoice.isAllowSubsetMatch() && coordinateChoice.isAllowSubsetMatch());
if (allowSubsetMatch && (submittedItems.size() > choiceItems.size())) {
boolean allChoiceItemsInSubmittedItems = true;
boolean allItemsInSubmittedWithoutSigFigs = true;
for (CoordinateItem choiceItem : choiceItems) {
boolean choiceItemInSubmittedItems = false;
boolean itemInSubmittedWithoutSigFigs = false;
for (CoordinateItem submittedItem : submittedItems) {
if (coordinateItemsMatch(submittedItem, choiceItem, sigFigsMin, sigFigsMax)) {
if (coordinateItemsMatch(submittedItem, choiceItem, sigFigsMin, sigFigsMax, false)) {
choiceItemInSubmittedItems = true;
break;
} else if (coordinateItemsMatch(submittedItem, choiceItem, sigFigsMin, sigFigsMax, true)) {
itemInSubmittedWithoutSigFigs = true;
}
}
if (!choiceItemInSubmittedItems) {
allChoiceItemsInSubmittedItems = false;
// Check if this is just a significant figures mismatch
if (!itemInSubmittedWithoutSigFigs) {
allItemsInSubmittedWithoutSigFigs = false;
}
break;
}
}
if (allChoiceItemsInSubmittedItems) {
responseCorrect = coordinateChoice.isCorrect();
feedback = (Content) coordinateChoice.getExplanation();
break;
} else if (allItemsInSubmittedWithoutSigFigs) {
feedback = new Content(DEFAULT_VALIDATION_RESPONSE);
feedback.setTags(new HashSet<>(ImmutableList.of("sig_figs", "sig_figs_too_many")));
break;
}
}
}
Expand All @@ -236,12 +262,19 @@ public final QuestionValidationResponse validateQuestionResponse(final Question
if (feedbackIsNullOrEmpty(feedback) && null != coordinateQuestion.getDefaultFeedback()) {
feedback = coordinateQuestion.getDefaultFeedback();
}
// If there was no default feedback, check for too few significant figures
if (feedbackIsNullOrEmpty(feedback) && submittedItems.stream().anyMatch(i -> i.getCoordinates().stream()
.anyMatch(c -> ValidationUtils.tooFewSignificantFigures(c, sigFigsMin, log)))) {
feedback = new Content(DEFAULT_VALIDATION_RESPONSE);
feedback.setTags(new HashSet<>(ImmutableList.of("sig_figs", "sig_figs_too_few")));
}

return new QuestionValidationResponse(question.getId(), answer, responseCorrect, feedback, new Date());
}

private boolean coordinateItemsMatch(final CoordinateItem submittedItem, final CoordinateItem choiceItem,
final Integer sigFigsMin, final Integer sigFigsMax) {
final Integer sigFigsMin, final Integer sigFigsMax,
final boolean allowTooManySigFigs) {

if (submittedItem.getCoordinates().size() != choiceItem.getCoordinates().size()) {
return false;
Expand All @@ -251,6 +284,14 @@ private boolean coordinateItemsMatch(final CoordinateItem submittedItem, final C
String submittedValue = submittedItem.getCoordinates().get(dimension);
String choiceValue = choiceItem.getCoordinates().get(dimension);

if (allowTooManySigFigs) {
// Check if the submission has more significant figures than the allowed maximum
if (null != sigFigsMax && ValidationUtils.tooManySignificantFigures(submittedValue, sigFigsMax, log)) {
// Check if the submission would match the choice if we ignore the excess significant figures
return ValidationUtils.numericValuesMatch(choiceValue, submittedValue, sigFigsMax, log);
}
}

int sigFigs = ValidationUtils.numberOfSignificantFiguresToValidateWith(submittedValue, sigFigsMin, sigFigsMax, log);
if (!ValidationUtils.numericValuesMatch(choiceValue, submittedValue, sigFigs, log)
|| null != sigFigsMin && ValidationUtils.tooFewSignificantFigures(submittedValue, sigFigsMin, log)
Expand Down
89 changes: 72 additions & 17 deletions src/test/java/uk/ac/cam/cl/dtg/isaac/quiz/IsaacCoordinateValidatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,15 @@ public class IsaacCoordinateValidatorTest {

private IsaacCoordinateValidator validator;
private IsaacCoordinateQuestion someCoordinateQuestion;
private IsaacCoordinateQuestion someUnorderedCoordinateQuestion;

private final CoordinateItem item1 = new CoordinateItem(List.of("1", "2"));
private final CoordinateItem item2 = new CoordinateItem(List.of("2", "1"));
private final CoordinateItem item3 = new CoordinateItem(List.of("1", "3"));
private final CoordinateItem item4 = new CoordinateItem(List.of("3", "1"));
private final CoordinateItem item2Again = new CoordinateItem(List.of("2", "1")); // Ensure no == comparisons.
private final CoordinateItem item1 = new CoordinateItem(List.of("1.0", "2.0"));
private final CoordinateItem item2 = new CoordinateItem(List.of("2.0", "1.0"));
private final CoordinateItem item3 = new CoordinateItem(List.of("1.0", "3.0"));
private final CoordinateItem item4 = new CoordinateItem(List.of("3.0", "1.0"));
private final CoordinateItem item2Again = new CoordinateItem(List.of("2.0", "1.0")); // Ensure no == comparisons.

private final CoordinateItem item1ExtraSigFig = new CoordinateItem(List.of("1.00", "2.00"));
private final CoordinateItem item1TooFewSigFigs = new CoordinateItem(List.of("1", "2"));

private final Content someIncorrectExplanation = new Content("Some incorrect explanation.");

Expand All @@ -58,13 +60,10 @@ public final void setUp() {
// Set up the question objects:
someCoordinateQuestion = new IsaacCoordinateQuestion();
someCoordinateQuestion.setNumberOfDimensions(2);
someCoordinateQuestion.setSignificantFiguresMin(1);
someCoordinateQuestion.setSignificantFiguresMax(1);
someCoordinateQuestion.setSignificantFiguresMin(2);
someCoordinateQuestion.setSignificantFiguresMax(2);
someCoordinateQuestion.setOrdered(true);

someUnorderedCoordinateQuestion = new IsaacCoordinateQuestion();
someUnorderedCoordinateQuestion.setNumberOfDimensions(2);

List<Choice> answerList = Lists.newArrayList();
ItemChoice someIncorrectChoice = new CoordinateChoice();
ItemChoice someCorrectChoice = new CoordinateChoice();
Expand All @@ -81,7 +80,6 @@ public final void setUp() {
answerList.add(someIncorrectChoice);
answerList.add(someCorrectChoice);
someCoordinateQuestion.setChoices(answerList);
someUnorderedCoordinateQuestion.setChoices(answerList);
}

@Test
Expand Down Expand Up @@ -170,36 +168,93 @@ public final void isaacCoordinateValidator_TestWrongNumberOfDimensions() {
assertTrue(response.getExplanation().getValue().contains("did not provide the expected number of dimensions"));
}

@Test
public final void isaacCoordinateValidator_TestTooManySignificantFigures() {
CoordinateChoice c = new CoordinateChoice();
c.setItems(List.of(item1ExtraSigFig, item2));

QuestionValidationResponse response = validator.validateQuestionResponse(someCoordinateQuestion, c);

assertFalse(response.isCorrect());
assertTrue(response.getExplanation().getTags().contains("sig_figs"));
assertTrue(response.getExplanation().getTags().contains("sig_figs_too_many"));
}

@Test
public final void isaacCoordinateValidator_TestTooFewSignificantFigures() {
CoordinateChoice c = new CoordinateChoice();
c.setItems(List.of(item1TooFewSigFigs, item2));

QuestionValidationResponse response = validator.validateQuestionResponse(someCoordinateQuestion, c);

assertFalse(response.isCorrect());
assertTrue(response.getExplanation().getTags().contains("sig_figs"));
assertTrue(response.getExplanation().getTags().contains("sig_figs_too_few"));
}

@Test
public final void isaacCoordinateValidator_TestSubsetOfCorrectChoice() {
someCoordinateQuestion.setOrdered(false);
CoordinateChoice c = new CoordinateChoice();
c.setItems(List.of(item1));

QuestionValidationResponse response = validator.validateQuestionResponse(someUnorderedCoordinateQuestion, c);
QuestionValidationResponse response = validator.validateQuestionResponse(someCoordinateQuestion, c);

assertFalse(response.isCorrect());
assertTrue(response.getExplanation().getValue().contains("but can you find more?"));
}

@Test
public final void isaacCoordinateValidator_TestSupersetOfSubsetMatchChoice() {
someCoordinateQuestion.setOrdered(false);
CoordinateChoice c = new CoordinateChoice();
c.setItems(List.of(item1, item3, item4));
QuestionValidationResponse response = validator.validateQuestionResponse(someUnorderedCoordinateQuestion, c);

QuestionValidationResponse response = validator.validateQuestionResponse(someCoordinateQuestion, c);

assertFalse(response.isCorrect());
assertEquals(someIncorrectExplanation, response.getExplanation());
}

@Test
public final void isaacCoordinateValidator_TestIncorrectSignificantFigures() {
CoordinateItem ci = new CoordinateItem(List.of("1.00", "2.00"));
public final void isaacCoordinateValidator_TestSubsetOfCorrectChoiceTooManySigFigs() {
someCoordinateQuestion.setOrdered(false);
CoordinateChoice c = new CoordinateChoice();
c.setItems(List.of(ci, item2));
c.setItems(List.of(item1ExtraSigFig));

QuestionValidationResponse response = validator.validateQuestionResponse(someCoordinateQuestion, c);

assertFalse(response.isCorrect());
assertTrue(response.getExplanation().getTags().contains("sig_figs"));
assertTrue(response.getExplanation().getTags().contains("sig_figs_too_many"));
}

@Test
public final void isaacCoordinateValidator_TestSupersetOfSubsetMatchChoiceTooManySigFigs() {
someCoordinateQuestion.setOrdered(false);
CoordinateChoice c = new CoordinateChoice();
c.setItems(List.of(item1ExtraSigFig, item3, item4));

QuestionValidationResponse response = validator.validateQuestionResponse(someCoordinateQuestion, c);

assertFalse(response.isCorrect());
assertTrue(response.getExplanation().getTags().contains("sig_figs"));
assertTrue(response.getExplanation().getTags().contains("sig_figs_too_many"));
}

@Test
public final void isaacCoordinateValidator_TestDefaultFeedback() {
// If default feedback is set, prefer it over "too few sig figs"
CoordinateChoice c = new CoordinateChoice();
c.setItems(List.of(item1TooFewSigFigs, item2));

Content defaultExplanation = new Content("Default feedback.");
someCoordinateQuestion.setDefaultFeedback(defaultExplanation);

QuestionValidationResponse response = validator.validateQuestionResponse(someCoordinateQuestion, c);

assertFalse(response.isCorrect());
assertEquals(defaultExplanation, response.getExplanation());
}

// Test the internals of the item-ordering:
Expand Down
Loading