Skip to content

Commit

Permalink
Fix bug with calculating quality evaluation
Browse files Browse the repository at this point in the history 
This patch improves the quality evaluation on a few points:
  - The stored value is no longer a calculated float. This helps us
    avoid floating point precision problems in the database.
  - The count is now correctly computed on full tree calculation.
    - Previously this was not the case and could lead to miscalculated
      averages after updates that happened after a full tree
      calculation.
  • Loading branch information
@jnatten
jnatten committed Jan 20, 2025
1 parent d7cafdb commit 540f2e4
Show file tree
Hide file tree
Showing 9 changed files with 245 additions and 60 deletions.
22 changes: 13 additions & 9 deletions src/main/java/no/ndla/taxonomy/domain/GradeAverage.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,23 +11,23 @@
import java.util.Optional;

public class GradeAverage {
public GradeAverage(double averageValue, int count) {
this.averageValue = averageValue;
public GradeAverage(int averageSum, int count) {
this.averageSum = averageSum;
this.count = count;
}

double averageValue;
int averageSum;
int count;

public static GradeAverage fromGrades(Collection<Optional<Grade>> grades) {
var existing = grades.stream().flatMap(Optional::stream).toList();
var count = existing.size();
var avg = existing.stream().mapToInt(Grade::toInt).average().orElse(0.0);
return new GradeAverage(avg, count);
var sum = existing.stream().mapToInt(Grade::toInt).sum();
return new GradeAverage(sum, count);
}

public double getAverageValue() {
return averageValue;
public int getAverageSum() {
return averageSum;
}

public int getCount() {
Expand All @@ -44,11 +44,15 @@ public boolean equals(Object obj) {
}

GradeAverage that = (GradeAverage) obj;
return Double.compare(that.averageValue, averageValue) == 0 && count == that.count;
return averageSum == that.averageSum && count == that.count;
}

@Override
public String toString() {
return "GradeAverage{averageValue=" + averageValue + ", count=" + count + '}';
return "GradeAverage{averageSum=" + averageSum + ", count=" + count + '}';
}

public double getAverageValue() {
return (double) averageSum / count;
}
}
64 changes: 30 additions & 34 deletions src/main/java/no/ndla/taxonomy/domain/Node.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,8 @@ public class Node extends DomainObject implements EntityWithMetadata {
@Column(name = "quality_evaluation_comment")
private String qualityEvaluationComment;

@Column(name = "child_quality_evaluation_average")
private Double childQualityEvaluationAverage;
@Column(name = "child_quality_evaluation_sum")
private int childQualityEvaluationSum;

@Column(name = "child_quality_evaluation_count")
private int childQualityEvaluationCount;
Expand Down Expand Up @@ -147,25 +147,33 @@ public Optional<String> getQualityEvaluationNote() {
}

public Optional<GradeAverage> getChildQualityEvaluationAverage() {
return Optional.ofNullable(this.childQualityEvaluationAverage)
.map(avg -> new GradeAverage(avg, this.childQualityEvaluationCount));
if (this.childQualityEvaluationSum == 0) {
return Optional.empty();
}
var gradeAverage = new GradeAverage(this.childQualityEvaluationSum, this.childQualityEvaluationCount);
return Optional.of(gradeAverage);
}

public void addGradeAverageTreeToAverageCalculation(GradeAverage newGradeAverage) {
var childAvg = getChildQualityEvaluationAverage();
if (childAvg.isEmpty()) {
this.childQualityEvaluationAverage = newGradeAverage.getAverageValue();
this.childQualityEvaluationSum = newGradeAverage.getAverageSum();
this.childQualityEvaluationCount = newGradeAverage.getCount();
return;
}

var oldSum = childAvg.get().getAverageValue() * childAvg.get().getCount();
var sumToAdd = newGradeAverage.getAverageValue() * newGradeAverage.getCount();
var oldSum = childAvg.get().getAverageSum();
var sumToAdd = newGradeAverage.getAverageSum();
var newSum = oldSum + sumToAdd;
var newCount = childAvg.get().getCount() + newGradeAverage.getCount();

this.childQualityEvaluationAverage = newSum / newCount;
this.childQualityEvaluationCount = newCount;
if (newCount > 0) {
this.childQualityEvaluationSum = newSum;
this.childQualityEvaluationCount = newCount;
} else {
this.childQualityEvaluationSum = 0;
this.childQualityEvaluationCount = 0;
}
}

public void removeGradeAverageTreeFromAverageCalculation(GradeAverage previousGradeAverage) {
Expand All @@ -178,17 +186,17 @@ public void removeGradeAverageTreeFromAverageCalculation(GradeAverage previousGr
return;
}

var totalSum = childAvg.get().getAverageValue() * childAvg.get().getCount();
var sumToRemove = previousGradeAverage.getAverageValue() * previousGradeAverage.getCount();
var totalSum = childAvg.get().getAverageSum();
var sumToRemove = previousGradeAverage.getAverageSum();

var newSum = totalSum - sumToRemove;
var newCount = childAvg.get().getCount() - previousGradeAverage.getCount();

if (newCount == 0) {
this.childQualityEvaluationAverage = null;
this.childQualityEvaluationSum = 0;
this.childQualityEvaluationCount = 0;
} else {
this.childQualityEvaluationAverage = newSum / newCount;
this.childQualityEvaluationSum = newSum;
this.childQualityEvaluationCount = newCount;
}
}
Expand All @@ -197,42 +205,30 @@ public void updateChildQualityEvaluationAverage(Optional<Grade> previousGrade, O
var childAvg = getChildQualityEvaluationAverage();
if (childAvg.isEmpty()) {
newGrade.ifPresent(ng -> {
this.childQualityEvaluationAverage = (double) ng.toInt();
this.childQualityEvaluationSum = ng.toInt();
this.childQualityEvaluationCount = 1;
});
return;
}

var avg = childAvg.get();

if (Double.isNaN(avg.averageValue)) {
logger.warn(
"Child quality evaluation average of node '{}' is NaN. Recalculating entire tree.",
this.getPublicId());
updateEntireAverageTree();
return;
}

if (previousGrade.isEmpty() && newGrade.isEmpty()) return;
else if (previousGrade.isEmpty()) { // New grade is present
var newCount = avg.getCount() + 1;
var newSum = ((avg.averageValue * avg.getCount()) + newGrade.get().toInt());
var newAverage = newSum / newCount;
var newSum = avg.averageSum + newGrade.get().toInt();
this.childQualityEvaluationCount = newCount;
this.childQualityEvaluationAverage = newAverage;
this.childQualityEvaluationSum = newSum;
} else if (newGrade.isEmpty()) { // Previous grade is present
var newCount = avg.getCount() - 1;
var oldSum = (avg.averageValue * avg.getCount());
var oldSum = avg.getAverageSum();
var newSum = oldSum - previousGrade.get().toInt();
var newAverage = newCount > 0 ? newSum / newCount : null;
this.childQualityEvaluationCount = newCount;
this.childQualityEvaluationAverage = newAverage;
this.childQualityEvaluationSum = newSum;
} else { // Both grades are present
var oldSum = avg.averageValue * avg.getCount();
var oldSum = avg.getAverageSum();
var newSum = oldSum - previousGrade.get().toInt() + newGrade.get().toInt();
var newAverage = newSum / avg.getCount();
this.childQualityEvaluationCount = avg.getCount();
this.childQualityEvaluationAverage = newAverage;
this.childQualityEvaluationSum = newSum;
}
}

Expand All @@ -246,10 +242,10 @@ public void updateEntireAverageTree() {
gradeAverage);

if (gradeAverage.count == 0) {
this.childQualityEvaluationAverage = null;
this.childQualityEvaluationSum = 0;
this.childQualityEvaluationCount = 0;
} else if (gradeAverage.count > 0) {
this.childQualityEvaluationAverage = gradeAverage.averageValue;
this.childQualityEvaluationSum = gradeAverage.getAverageSum();
this.childQualityEvaluationCount = gradeAverage.count;
}
}
Expand Down
4 changes: 4 additions & 0 deletions src/main/java/no/ndla/taxonomy/domain/UpdateOrDelete.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@ public static <T> UpdateOrDelete<T> Default() {
return new UpdateOrDelete<>();
}

public static <T> UpdateOrDelete<T> Update(T value) {
return new UpdateOrDelete<>(value, false);
}

public boolean isDelete() {
return delete;
}
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/no/ndla/taxonomy/repositories/NodeRepository.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public interface NodeRepository extends TaxonomyRepository<Node> {
@Query(
"""
UPDATE Node n
SET n.childQualityEvaluationAverage = NULL,
SET n.childQualityEvaluationSum = 0,
n.childQualityEvaluationCount = 0
""")
void wipeQualityEvaluationAverages();
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/no/ndla/taxonomy/rest/v1/CrudController.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ protected void deleteEntity(@PathVariable("id") URI id) {

if (parents.isPresent()) {
var p = parents.get();
qualityEvaluationService.updateQualityEvaluationOf(p, oldGrade, Optional.empty());
qualityEvaluationService.updateQualityEvaluationOfRecursive(p, oldGrade, Optional.empty());
}
}

Expand Down
23 changes: 14 additions & 9 deletions src/main/java/no/ndla/taxonomy/service/QualityEvaluationService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@

package no.ndla.taxonomy.service;

import jakarta.persistence.EntityManager;
import jakarta.persistence.PersistenceContext;
import java.net.URI;
import java.util.Collection;
import java.util.Optional;
Expand All @@ -27,6 +29,9 @@ public class QualityEvaluationService {

private final NodeRepository nodeRepository;

@PersistenceContext
private EntityManager entityManager;

public QualityEvaluationService(NodeRepository nodeRepository) {
this.nodeRepository = nodeRepository;
}
Expand Down Expand Up @@ -96,17 +101,11 @@ protected void updateQualityEvaluationOfParents(Node node, Optional<Grade> oldGr
return;
}

updateQualityEvaluationOf(node.getParentNodes(), oldGrade, newGrade);
}

@Transactional
public void updateQualityEvaluationOf(
Collection<Node> parents, Optional<Grade> oldGrade, Optional<Grade> newGrade) {
updateQualityEvaluationOfRecursive(parents, oldGrade, newGrade);
updateQualityEvaluationOfRecursive(node.getParentNodes(), oldGrade, newGrade);
}

@Transactional
protected void updateQualityEvaluationOfRecursive(
public void updateQualityEvaluationOfRecursive(
Collection<Node> parents, Optional<Grade> oldGrade, Optional<Grade> newGrade) {
var updatedParents = parents.stream()
.peek(p -> {
Expand All @@ -128,9 +127,15 @@ public void updateEntireAverageTreeForNode(URI publicId) {
nodeRepository.save(node);
}

public void wipeQualityEvaluationAverages() {
nodeRepository.wipeQualityEvaluationAverages();
nodeRepository.flush();
entityManager.clear();
}

@Transactional
public void updateQualityEvaluationOfAllNodes() {
nodeRepository.wipeQualityEvaluationAverages();
wipeQualityEvaluationAverages();
var nodeStream = nodeRepository.findNodesWithQualityEvaluation();
nodeStream.forEach(
node -> updateQualityEvaluationOfParents(node, Optional.empty(), node.getQualityEvaluationGrade()));
Expand Down
3 changes: 2 additions & 1 deletion src/main/java/no/ndla/taxonomy/service/dtos/GradeAverageDTO.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ public static double roundToSingleDecimal(double value) {

public static Optional<GradeAverageDTO> fromNode(Node node) {
return node.getChildQualityEvaluationAverage().map(ga -> {
var roundedAvg = roundToSingleDecimal(ga.getAverageValue());
var avg = (double) ga.getAverageSum() / ga.getCount();
var roundedAvg = roundToSingleDecimal(avg);
var count1 = ga.getCount();
return new GradeAverageDTO(roundedAvg, count1);
});
Expand Down
7 changes: 7 additions & 0 deletions src/main/resources/db-master-changelog.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1602,4 +1602,11 @@
</sql>
</changeSet>

<changeSet id="20250120 Quality evaluation sum instead of calculated" author="Jonas Natten">
<dropColumn tableName="node" columnName="child_quality_evaluation_average"/>
<addColumn tableName="node">
<column name="child_quality_evaluation_sum" type="int" defaultValueNumeric="0" />
</addColumn>
</changeSet>

</databaseChangeLog>
Loading

0 comments on commit 540f2e4

Please sign in to comment.