Skip to content
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

Average grades in export #2366

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
152 changes: 114 additions & 38 deletions evap/results/exporters.py
Original file line number Diff line number Diff line change
Expand Up @@ -112,24 +112,29 @@ def filter_text_and_heading_questions(questions: Iterable[Question]) -> list[Que
return filtered_questions

@staticmethod
def filter_evaluations(
semesters: Iterable[Semester],
evaluation_states: Iterable[Evaluation.State],
program_ids: Iterable[int],
course_type_ids: Iterable[int],
contributor: UserProfile | None,
include_not_enough_voters: bool,
def filter_evaluations( # noqa: PLR0912
semesters: Iterable[Semester] | None = None,
evaluation_states: Iterable[Evaluation.State] | None = None,
program_ids: Iterable[int] | None = None,
course_type_ids: Iterable[int] | None = None,
contributor: UserProfile | None = None,
include_not_enough_voters: bool = False,
) -> tuple[list[tuple[Evaluation, OrderedDict[int, list[QuestionResult]]]], list[Questionnaire], bool]:
# pylint: disable=too-many-locals
course_results_exist = False
evaluations_with_results = []
used_questionnaires: set[Questionnaire] = set()
evaluations_filter = Q(
course__semester__in=semesters,
state__in=evaluation_states,
course__programs__in=program_ids,
course__type__in=course_type_ids,
)

evaluations_filter = Q()
if semesters:
evaluations_filter &= Q(course__semester__in=semesters)
if evaluation_states:
evaluations_filter &= Q(state__in=evaluation_states)
if program_ids:
evaluations_filter &= Q(course__programs__in=program_ids)
if course_type_ids:
evaluations_filter &= Q(course__type__in=course_type_ids)
Comment on lines +129 to +136
Copy link
Member

Choose a reason for hiding this comment

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

The ifs should all use is not None checks to distuingish between empty iterable and None.

Copy link
Collaborator Author

@fidoriel fidoriel Jan 27, 2025

Choose a reason for hiding this comment

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

But then empty iterables are not covered, and the code breaks, we want to skip both.

Copy link
Member

Choose a reason for hiding this comment

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

What do you mean by "not covered"? If I put in evaluation_states=[], I expect that the result is empty. I don't think we are relying on this behavior anywhere, and the is not None version seems more intuitive


if contributor:
evaluations_filter = evaluations_filter & (
Q(course__responsibles__in=[contributor]) | Q(contributions__contributor__in=[contributor])
Expand Down Expand Up @@ -198,6 +203,8 @@ def write_headings_and_evaluation_info(
else:
self.write_cell(export_name, "headline")

self.write_cell(_("Average for this Question"), "evaluation")

for evaluation, __ in evaluations_with_results:
title = evaluation.full_name
if len(semesters) > 1:
Expand All @@ -208,17 +215,19 @@ def write_headings_and_evaluation_info(

self.next_row()
self.write_cell(_("Programs"), "bold")
self.write_cell("", "program")
for evaluation, __ in evaluations_with_results:
self.write_cell("\n".join([d.name for d in evaluation.course.programs.all()]), "program")

self.next_row()
self.write_cell(_("Course Type"), "bold")
self.write_cell("", "program")
for evaluation, __ in evaluations_with_results:
self.write_cell(evaluation.course.type.name, "border_left_right")

self.next_row()
# One more cell is needed for the question column
self.write_empty_row_with_styles(["default"] + ["border_left_right"] * len(evaluations_with_results))
# One column for the question, one column for the average, n columns for the evaluations
self.write_empty_row_with_styles(["default"] + ["border_left_right"] * (len(evaluations_with_results) + 1))

def write_overall_results(
self,
Expand All @@ -228,14 +237,17 @@ def write_overall_results(
annotated_evaluations = [e for e, __ in evaluations_with_results]

self.write_cell(_("Overall Average Grade"), "bold")
self.write_cell("", "border_left_right")
averages = (distribution_to_grade(calculate_average_distribution(e)) for e in annotated_evaluations)
self.write_row(averages, lambda avg: self.grade_to_style(avg) if avg else "border_left_right")

self.write_cell(_("Total voters/Total participants"), "bold")
self.write_cell("", "total_voters")
voter_ratios = (f"{e.num_voters}/{e.num_participants}" for e in annotated_evaluations)
self.write_row(voter_ratios, style="total_voters")

self.write_cell(_("Evaluation rate"), "bold")
self.write_cell("", "evaluation_rate")
# round down like in progress bar
participant_percentages = (
f"{int((e.num_voters / e.num_participants) * 100) if e.num_participants > 0 else 0}%"
Expand All @@ -247,19 +259,21 @@ def write_overall_results(
# Only query the number of evaluations once and keep track of it here.
count_gt_1: list[bool] = [e.course_evaluations_count > 1 for e in annotated_evaluations]

# Borders only if there is a course grade below. Offset by one column
# Borders only if there is a course grade below. Offset by one column for column title and one for average
self.write_empty_row_with_styles(
["default"] + ["border_left_right" if gt1 else "default" for gt1 in count_gt_1]
["default", "default"] + ["border_left_right" if gt1 else "default" for gt1 in count_gt_1]
)

self.write_cell(_("Evaluation weight"), "bold")
weight_percentages = (
self.write_cell("")
weight_percentages = tuple(
Copy link
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Collaborator Author

@fidoriel fidoriel Jan 27, 2025

Choose a reason for hiding this comment

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

We need to write an empty cell for the average col

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tuple was something with typing.

Copy link
Member

Choose a reason for hiding this comment

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

The expected type below is Iterable, so tuple shouldn't be necessary

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should introduce a style "missing_average" or so to make these write_cell calls more clear? It could be identical to another style, but just to make it more readable

f"{e.weight_percentage}%" if gt1 else None
for e, gt1 in zip(annotated_evaluations, count_gt_1, strict=True)
)
self.write_row(weight_percentages, lambda s: "evaluation_weight" if s is not None else "default")

self.write_cell(_("Course Grade"), "bold")
self.write_cell("")
for evaluation, gt1 in zip(annotated_evaluations, count_gt_1, strict=True):
if not gt1:
self.write_cell()
Expand All @@ -271,58 +285,116 @@ def write_overall_results(
self.next_row()

# Same reasoning as above.
self.write_empty_row_with_styles(["default"] + ["border_top" if gt1 else "default" for gt1 in count_gt_1])
self.write_empty_row_with_styles(
["default", "default"] + ["border_top" if gt1 else "default" for gt1 in count_gt_1]
)

@classmethod
def _calculate_display_result(
Copy link
Member

Choose a reason for hiding this comment

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

Maybe _get_average_grade_and_approval_ratio?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

think _calculate_display_result is accurate what it does. Maybe _calculate_evaluation_ratio and _calculate_evaluation_average_ratio?

cls, questionnaire_id: int, question: Question, results: OrderedDict[int, list[QuestionResult]]
) -> tuple[float | None, float | None]:
value_sum = 0.0
count_sum = 0
approval_count = 0

for grade_result in results[questionnaire_id]:
if grade_result.question.id != question.id or not RatingResult.has_answers(grade_result):
continue

value_sum += grade_result.average * grade_result.count_sum
count_sum += grade_result.count_sum
if grade_result.question.is_yes_no_question:
approval_count += grade_result.approval_count

if not value_sum:
return None, None

avg = value_sum / count_sum
if question.is_yes_no_question:
average_approval_ratio = approval_count / count_sum if count_sum > 0 else 0
return avg, average_approval_ratio
return avg, None

@classmethod
def _calculate_display_result_average(
Copy link
Member

Choose a reason for hiding this comment

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

Technically should also be called _get_average_grade_and_approval_ratio. Given that this is also the name I suggested for the function above: Why do we need two separate functions? They have a very similar structure, I feel like we could have one function that can be used for both computations, we're just applying a different filter.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

One is using the other function. They look very similar but do very different things.

Copy link
Member

@niklasmohrin niklasmohrin Feb 3, 2025

Choose a reason for hiding this comment

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

The second one is an average of averages (why?). I also don't like the current names, I don't know what a "display result average" is, let's emphasize what the sample space of this average is

Copy link
Member

Choose a reason for hiding this comment

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

My thought here was that with addition being commutative and division being distributive, the "average of averages" should just be like computing the 1-level-average correctly.

I now see that the average-of-averages approach has different weights (-> it doesn't matter that on evaluation1, 50 people voted, while it was 200 people on evaluation2), which probably makes sense for the number we show in the exporter, so I'm fine with keeping the two methods. Agree with @niklasmohrin on the naming, I could see _get_average_of_average_grade_and_approval_ratio working, don't know if we have something better.

cls,
evaluations_with_results: list[tuple[Evaluation, OrderedDict[int, list[QuestionResult]]]],
questionnaire_id: int,
question: Question,
) -> tuple[float | None, float | None]:
avg_value_sum = 0.0
count_avg = 0
avg_approval_sum = 0.0
count_approval = 0

for __, results in evaluations_with_results:
if (
results.get(questionnaire_id) is None
): # we iterate over all distinct questionaires from all evaluations but some evaluations do not include a specific questionaire
continue
avg, average_approval_ratio = cls._calculate_display_result(questionnaire_id, question, results)
if avg is not None:
avg_value_sum += avg
count_avg += 1
if average_approval_ratio is not None:
avg_approval_sum += average_approval_ratio
count_approval += 1

return avg_value_sum / count_avg if count_avg else None, (
avg_approval_sum / count_approval if count_approval else None
)

def write_questionnaire(
self,
questionnaire: Questionnaire,
evaluations_with_results: list[tuple[Evaluation, OrderedDict[int, list[QuestionResult]]]],
contributor: UserProfile | None,
all_evaluations_with_results: list[tuple[Evaluation, OrderedDict[int, list[QuestionResult]]]],
) -> None:
if contributor and questionnaire.type == Questionnaire.Type.CONTRIBUTOR:
self.write_cell(f"{questionnaire.public_name} ({contributor.full_name})", "bold")
else:
self.write_cell(questionnaire.public_name, "bold")

self.write_cell("", "border_left_right")

# first cell of row is printed above
self.write_empty_row_with_styles(["border_left_right"] * len(evaluations_with_results))

for question in self.filter_text_and_heading_questions(questionnaire.questions.all()):
self.write_cell(question.text, "italic" if question.is_heading_question else "default")

average_grade, approval_ratio = self._calculate_display_result_average(
all_evaluations_with_results, questionnaire.id, question
)
if approval_ratio is not None and average_grade is not None:
self.write_cell(f"{approval_ratio:.0%}", self.grade_to_style(average_grade))
elif average_grade is not None:
self.write_cell(average_grade, self.grade_to_style(average_grade))
else:
self.write_cell("", "border_left_right")

# evaluations
for __, results in evaluations_with_results:
if questionnaire.id not in results or question.is_heading_question:
self.write_cell(style="border_left_right")
continue

values = []
count_sum = 0
approval_count = 0

for grade_result in results[questionnaire.id]:
if grade_result.question.id != question.id or not RatingResult.has_answers(grade_result):
continue

values.append(grade_result.average * grade_result.count_sum)
count_sum += grade_result.count_sum
if grade_result.question.is_yes_no_question:
approval_count += grade_result.approval_count
avg, average_approval_ratio = self._calculate_display_result(questionnaire.id, question, results)

if not values:
if avg is None:
self.write_cell(style="border_left_right")
continue

avg = sum(values) / count_sum
if question.is_yes_no_question:
percent_approval = approval_count / count_sum if count_sum > 0 else 0
self.write_cell(f"{percent_approval:.0%}", self.grade_to_style(avg))
self.write_cell(f"{average_approval_ratio:.0%}", self.grade_to_style(avg))
else:
self.write_cell(avg, self.grade_to_style(avg))
self.next_row()

self.write_empty_row_with_styles(["default"] + ["border_left_right"] * len(evaluations_with_results))
self.write_empty_row_with_styles(["default"] + ["border_left_right"] * (len(evaluations_with_results) + 1))

# pylint: disable=arguments-differ
# pylint: disable=arguments-differ,too-many-locals
def export_impl(
self,
semesters: QuerySetOrSequence[Semester],
Expand All @@ -335,6 +407,8 @@ def export_impl(
# We want to throw early here, since workbook.save() will throw an IndexError otherwise.
assert len(selection_list) > 0

all_evaluations_with_results, _, _ = self.filter_evaluations(evaluation_states=[Evaluation.State.PUBLISHED])

for sheet_counter, (program_ids, course_type_ids) in enumerate(selection_list, 1):
self.cur_sheet = self.workbook.add_sheet("Sheet " + str(sheet_counter))
self.cur_row = 0
Expand All @@ -358,7 +432,9 @@ def export_impl(
)

for questionnaire in used_questionnaires:
self.write_questionnaire(questionnaire, evaluations_with_results, contributor)
self.write_questionnaire(
questionnaire, evaluations_with_results, contributor, all_evaluations_with_results
)

self.write_overall_results(evaluations_with_results, course_results_exist)

Expand Down
Loading
Loading