Skip to content

Commit

Permalink
implement annotation merging #41
Browse files Browse the repository at this point in the history
  • Loading branch information
Luro02 committed Sep 24, 2024
1 parent c6b9104 commit be43560
Show file tree
Hide file tree
Showing 9 changed files with 460 additions and 11 deletions.
2 changes: 1 addition & 1 deletion pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@
<jackson.version>2.17.2</jackson.version>
<slf4j.version>2.0.16</slf4j.version>
<okhttp.version>4.12.0</okhttp.version>
<autograder.version>0.6.1</autograder.version>
<autograder.version>0.6.2</autograder.version>
<jgit.version>7.0.0.202409031743-r</jgit.version>
<junit.version>5.11.0</junit.version>
<versions-maven-plugin.version>2.17.1</versions-maven-plugin.version>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
/* Licensed under EPL-2.0 2024. */
package edu.kit.kastel.sdq.artemis4j.grading;

import java.util.ArrayList;
import java.util.List;
import java.util.Objects;
import java.util.Optional;
import java.util.UUID;
Expand All @@ -24,6 +26,17 @@ public final class Annotation {
private final AnnotationSource source;
private String customMessage;
private Double customScore;
// If not empty, this list contains classifiers that are used to group annotations.
// For example, all annotations that are related, could have the classifier ["a"],
// then they would be grouped together.
//
// You can add further classifiers to group annotations in a more fine-grained way:
// For example, when you have annotations with the classifiers ["a", "b"]
// and ["a", "c"], then if there are more than "annotationLimit"
// with the classifier "a", it would merge all annotations with the classifiers ["a", "b"]
// and all annotations with the classifiers ["a", "c"].
private final List<String> classifiers;
private final Integer annotationLimit;

/**
* Deserializes an annotation from its metajson format
Expand All @@ -37,6 +50,8 @@ public Annotation(AnnotationDTO dto, MistakeType mistakeType) {
this.source = dto.source() != null ? dto.source() : AnnotationSource.UNKNOWN;
this.customMessage = dto.customMessageForJSON();
this.customScore = dto.customPenaltyForJSON();
this.classifiers = dto.classifiers();
this.annotationLimit = dto.annotationLimit();
}

Annotation(
Expand All @@ -47,6 +62,19 @@ public Annotation(AnnotationDTO dto, MistakeType mistakeType) {
String customMessage,
Double customScore,
AnnotationSource source) {
this(mistakeType, filePath, startLine, endLine, customMessage, customScore, source, List.of(), null);
}

Annotation(
MistakeType mistakeType,
String filePath,
int startLine,
int endLine,
String customMessage,
Double customScore,
AnnotationSource source,
List<String> classifiers,
Integer annotationLimit) {
// Validate custom penalty and message
if (mistakeType.isCustomAnnotation()) {
if (customScore == null) {
Expand All @@ -67,6 +95,8 @@ public Annotation(AnnotationDTO dto, MistakeType mistakeType) {
this.customMessage = customMessage;
this.customScore = customScore;
this.source = source;
this.classifiers = new ArrayList<>(classifiers);
this.annotationLimit = annotationLimit;
}

/**
Expand Down Expand Up @@ -134,6 +164,26 @@ public void setCustomMessage(String message) {
this.customMessage = message;
}

/**
* Returns the classifiers of this annotation that can be used to group annotations.
*
* @return a list of classifiers
*/
public List<String> getClassifiers() {
return new ArrayList<>(this.classifiers);
}

/**
* Returns the maximum number of annotations that should be displayed if one or more classifiers match.
* <p>
* It is up to the implementation, how the limit is applied in case of multiple classifiers.
*
* @return the maximum number of annotations that should be displayed
*/
public Optional<Integer> getAnnotationLimit() {
return Optional.ofNullable(this.annotationLimit);
}

/**
* The custom score associated with this message, if any. Is always empty for
* predefined annotations, and never empty for custom annotations.
Expand Down Expand Up @@ -161,7 +211,17 @@ public AnnotationSource getSource() {
* Serializes this annotation to its metajson format
*/
public AnnotationDTO toDTO() {
return new AnnotationDTO(uuid, type.getId(), startLine, endLine, filePath, customMessage, customScore, source);
return new AnnotationDTO(
uuid,
type.getId(),
startLine,
endLine,
filePath,
customMessage,
customScore,
source,
classifiers,
annotationLimit);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,176 @@
/* Licensed under EPL-2.0 2024. */
package edu.kit.kastel.sdq.artemis4j.grading;

import java.text.MessageFormat;
import java.util.ArrayList;
import java.util.Collection;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.StringJoiner;
import java.util.stream.Collectors;

import edu.kit.kastel.sdq.artemis4j.i18n.FormatString;

/**
* Merges annotations based on their classifiers.
*/
final class AnnotationMerger {
private static final FormatString MERGED_ANNOTATIONS_FORMAT = new FormatString(
new MessageFormat("{0}Weitere Probleme in {1}.", Locale.GERMAN),
Map.of(Locale.ENGLISH, new MessageFormat("{0}Other problems in {1}.", Locale.ENGLISH)));

private AnnotationMerger() {}

/**
* Merges annotations based on their classifiers or not if they have none.
* <p>
* This method assumes that the annotation uuids are unique.
*
* @param unreducedAnnotations the list of annotations to merge
* @param upperAnnotationLimit the maximum number of annotations for the first classifier
* @param locale the locale to use for the format string
* @return the merged list of annotations
*/
static List<Annotation> mergeAnnotations(
Collection<Annotation> unreducedAnnotations, int upperAnnotationLimit, Locale locale) {
// -1 means no limit (useful for unit tests, where one wants to see all annotations)
if (upperAnnotationLimit == -1) {
return new ArrayList<>(unreducedAnnotations);
}

// first group all problems by the first classifier:
Map<String, List<Annotation>> groupedAnnotations = unreducedAnnotations.stream()
.collect(Collectors.groupingBy(
annotation -> annotation.getClassifiers().stream().findFirst().orElse(annotation.getUUID()),
LinkedHashMap::new,
Collectors.toList()));

List<Annotation> result = new ArrayList<>();
for (List<Annotation> annotationsForClassifier : groupedAnnotations.values()) {
// if the annotation limit is set, use it (if it does not exceed the upper limit),
// otherwise use the upper limit
int targetNumberOfAnnotations = Math.min(
upperAnnotationLimit,
annotationsForClassifier.get(0).getAnnotationLimit().orElse(upperAnnotationLimit));

if (annotationsForClassifier.size() <= targetNumberOfAnnotations) {
result.addAll(annotationsForClassifier);
continue;
}

// Further partition the annotations by their remaining classifiers:
annotationsForClassifier.stream()
.collect(Collectors.groupingBy(
annotation -> {
List<String> classifiers = annotation.getClassifiers();
if (classifiers.size() <= 1) {
return annotation.getUUID();
} else {
// to simplify the grouping code, we merge the remaining classifiers
// into a single string
return String.join(" ", classifiers.subList(1, classifiers.size()));
}
},
LinkedHashMap::new,
Collectors.toList()))
.values()
.stream()
.flatMap(list -> merge(list, targetNumberOfAnnotations, locale).stream())
.forEach(result::add);
}

return result;
}

private static List<Annotation> merge(List<Annotation> annotations, int limit, Locale locale) {
// use a dumb algorithm: keep the first limit - 1 annotations, and merge the remainder into a single annotation
if (annotations.size() <= limit) {
return annotations;
}

List<Annotation> result = new ArrayList<>(annotations.subList(0, limit - 1));
List<Annotation> toMerge = annotations.subList(limit - 1, annotations.size());

// first we try to find an annotation with a custom message, this is the main one that will be shown to the user
int firstIndexWithCustomMessage = 0;
for (int i = 0; i < toMerge.size(); i++) {
if (toMerge.get(i).getCustomMessage().isPresent()) {
firstIndexWithCustomMessage = i;
break;
}
}

// the first annotation with the custom message is removed, so it doesn't appear twice
Annotation firstAnnotation = toMerge.remove(firstIndexWithCustomMessage);
// if there are no annotations with a custom message, the firstIndexWithCustomMessage will be 0,
// and because it doesn't have a custom message it will be null
String message = firstAnnotation.getCustomMessage().orElse(null);
if (message == null) {
message = "";
} else {
// some messages might not end with a period, which would look weird with the above format string,
// so this adds one if necessary
if (!message.endsWith(".")) {
message += ".";
}

message += " ";
}

String customMessage = MERGED_ANNOTATIONS_FORMAT
.format(message, displayLocations(firstAnnotation, toMerge))
.translateTo(locale);

result.add(new Annotation(
firstAnnotation.getMistakeType(),
firstAnnotation.getFilePath(),
firstAnnotation.getStartLine(),
firstAnnotation.getEndLine(),
customMessage,
firstAnnotation.getCustomScore().orElse(null),
firstAnnotation.getSource()));

return result;
}

private static String displayLocations(Annotation first, Collection<Annotation> others) {
Map<String, List<Annotation>> positionsByFile = others.stream()
.collect(Collectors.groupingBy(Annotation::getFilePath, LinkedHashMap::new, Collectors.toList()));

// if all annotations are in the same file, we don't need to display the filename
boolean withoutFilename = positionsByFile.size() == 1 && positionsByFile.containsKey(first.getFilePath());

StringJoiner joiner = new StringJoiner(", ");
// Format should look like this: File:(L1, L2, L3), File2:(L4, L5), File3:L5
for (Map.Entry<String, List<Annotation>> entry : positionsByFile.entrySet()) {
String path = entry.getKey();
List<Annotation> filePositions = entry.getValue();

String lines = filePositions.stream()
.map(position -> "L%d".formatted(position.getStartLine()))
.collect(Collectors.joining(", "));

if (filePositions.size() > 1 && !withoutFilename) {
lines = "(%s)".formatted(lines);
}

if (withoutFilename) {
joiner.add(lines);
continue;
}

joiner.add("%s:%s".formatted(getFilenameWithoutExtension(path), lines));
}

return joiner.toString();
}

private static String getFilenameWithoutExtension(String path) {
String[] parts = path.split("[\\\\\\/]");
String file = parts[parts.length - 1];

return file.split("\\.")[0];
}
}
32 changes: 28 additions & 4 deletions src/main/java/edu/kit/kastel/sdq/artemis4j/grading/Assessment.java
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
public class Assessment extends ArtemisConnectionHolder {
private static final Logger log = org.slf4j.LoggerFactory.getLogger(Assessment.class);

private static final int DEFAULT_ANNOTATION_LIMIT = 12;
private static final FormatString MANUAL_FEEDBACK = new FormatString(new MessageFormat("[{0}:{1}] {2}"));
private static final FormatString MANUAL_FEEDBACK_CUSTOM_EXP =
new FormatString(new MessageFormat("[{0}:{1}] " + "{2}\nExplanation: {3}"));
Expand Down Expand Up @@ -187,10 +188,25 @@ public Annotation addCustomAnnotation(
}

public Annotation addAutograderAnnotation(
MistakeType mistakeType, String filePath, int startLine, int endLine, String explanation) {
MistakeType mistakeType,
String filePath,
int startLine,
int endLine,
String explanation,
String checkName,
String problemType,
Integer annotationLimit) {
Double customScore = mistakeType.isCustomAnnotation() ? 0.0 : null;
var annotation = new Annotation(
mistakeType, filePath, startLine, endLine, explanation, customScore, AnnotationSource.AUTOGRADER);
mistakeType,
filePath,
startLine,
endLine,
explanation,
customScore,
AnnotationSource.AUTOGRADER,
List.of(checkName, problemType),
annotationLimit);
this.annotations.add(annotation);
return annotation;
}
Expand Down Expand Up @@ -308,7 +324,7 @@ public double getMaxPoints() {
* Calculates the points from all annotations of a specific mistake type.
*
* @return Empty, if no annotations of this type are present. Otherwise, the
* total points for the annotations.
* total points for the annotations.
*/
public Optional<Points> calculatePointsForMistakeType(MistakeType mistakeType) {
var annotationsWithType = this.getAnnotations(mistakeType);
Expand Down Expand Up @@ -386,11 +402,19 @@ private List<FeedbackDTO> packAssessmentForArtemis() throws AnnotationMappingExc
List<FeedbackDTO> feedbacks = new ArrayList<>(
this.testResults.stream().map(TestResult::getDto).toList());

// The autograder creates many annotations, because it highlights every single violation.
// For Artemis, we want to group these annotations if they are more than a certain number.
//
// The code is mainly intended to group annotations created by the autograder, but will work
// for any annotation that has the classifiers set.
List<Annotation> allAnnotations =
AnnotationMerger.mergeAnnotations(this.annotations, DEFAULT_ANNOTATION_LIMIT, this.studentLocale);

// For each annotation we have a manual feedback at the respective line
// These feedbacks deduct no points. They are just for the student to see in the
// Artemis code viewer
// We group annotations by file and line to have at most one annotation per line
feedbacks.addAll(this.annotations.stream()
feedbacks.addAll(allAnnotations.stream()
.collect(
Collectors.groupingBy(Annotation::getFilePath, Collectors.groupingBy(Annotation::getStartLine)))
.entrySet()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,17 @@ public int hashCode() {
return Objects.hashCode(this.getId());
}

private Optional<ResultDTO> getRelevantResult() {
/**
* Returns the relevant result for this submission.
* <p>
* The difference between this method and {@link #getLatestResult()} is that
* when a submission has multiple results from different correction rounds, this
* method will return the result for the current correction round. If you want the
* latest result regardless of the correction round, use {@link #getLatestResult()}.
*
* @return the relevant result, if present
*/
public Optional<ResultDTO> getRelevantResult() {
var results = this.dto.nonAutomaticResults();

if (results.isEmpty()) {
Expand Down
Loading

0 comments on commit be43560

Please sign in to comment.