Average grades in export #2366

fidoriel · 2025-01-13T17:46:50Z

closes #2001

evap/results/exporters.py

niklasmohrin

Seems mostly correct, but I think we should make an effort to keep the structure of these methods clear

richardebeling · 2025-02-24T22:06:38Z

evap/results/tests/test_exporters.py

+
+        self.assertEqual(
+            float(workbook.sheets()[0].row_values(5)[1]),
+            (float(workbook.sheets()[0].row_values(5)[2]) + float(workbook.sheets()[0].row_values(5)[3])) / 2,


From a quick glance, it looks like the setup above creates the same exact average result for both evaluations and same count of answers inside of them. This doesn't test whether we compute the average-of-averages with correct weighting. Can we change them to have different answer counts?

Coincidentally, float-equality here made me wonder about correctness. It should work if the two values here are identical, the sum is then 2 * the original value, which is divisible by 2 to perfectly recreate the original value. However, I'd like the test to test a somewhat more sophisticated case. We'll likely need to use assertAlmostEqual then.

@fidoriel This currently compares a value the system produces against another value that the system produces, which leaves the chance that both might be wrong, but the test still passes.

For a bit more assertiveness, I would appreciate if we could change the assertion here to the exact string that should be shown in the output, optimally in the string-domain (without to-number conversion)

evap/results/tests/test_exporters.py

evap/results/exporters.py

evap/staff/views.py

evap/results/tests/test_exporters.py

evap/results/exporters.py

evap/results/tests/test_exporters.py

evap/results/exporters.py

janno42

📊

niklasmohrin

The output looks correct, but I think we need to spend some more time on the code and testing, otherwise it will become hard to maintain the exporter

niklasmohrin · 2025-08-03T17:06:26Z

evap/evaluation/tools.py

 class ExcelExporter(ABC):
    styles = {
        "default": xlwt.Style.default_style,
+        "missing_average": xlwt.Style.default_style,


both styles should be defined in the same place, so this should be moved to the other class

niklasmohrin · 2025-08-03T17:29:21Z

evap/results/exporters.py

+    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,


I don't think that any of these should have defaults, there are two callers and both of them know exactly what they want to filter for.

I was wondering if we could get away without None by passing in Semester.objects.all() which results in something like WHERE "evaluation_course"."semester_id" IN (SELECT "evaluation_semester"."id" FROM "evaluation_semester") - I guess the database should be able to see that this check can be avoided @richardebeling ? But having None is also fine for now, I just think that the caller should be explicit about this

For contributor and include_not_enough_voters, I am not even sure if the current version is correct - wouldn't we want to use answers across all contributors for the average? Currently, we only use answers to the general questionnaire. And I guess include_not_enough_voters should be true for the average (@janno42 ?)

I'm confused. Aren't there average values for contributor questions in the current version of this PR? I thought so.

But yes, include_not_enough_voters should be True for the averages.

What I take from https://github.com/fidoriel/EvaP/blob/8a736d0713823104b69910edfbf23cfe49ab1ce4/evap/results/exporters.py#L415 is that even when contributor is not None we use the default of None for filter_evaluations

niklasmohrin · 2025-08-03T17:33:16Z

evap/results/exporters.py

+        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):


Just to be sure, we want to have the same average in all sheets and not the average for the programs and course types of that sheet for every sheet? @janno42

Now that you mention it... I think I wouldn't add this filter if we'd just have different course types, because their questions often differ. But for programs it might actually be important to have different averages. And then, when we're already at it, we can do the separation for both.
So yes, please calculate the averages for evaluations within the programs and course types of that sheet only.

niklasmohrin · 2025-08-03T17:43:34Z

evap/results/exporters.py

+    def _get_average_grade_and_approval_ratio(
+        cls, questionnaire_id: int, question: Question, results: OrderedDict[int, list[QuestionResult]]


This uses questionnaire_id and results only once to do results[questionnaire_id], so we should just do that in the caller

niklasmohrin · 2025-08-03T17:46:15Z

evap/results/exporters.py

+            if grade_result.question.id != question.id or not RatingResult.has_answers(grade_result):
+                continue
+
+            value_sum += grade_result.average * grade_result.count_sum


I think it would be fair to just ask the AnsweredRatingResult for the sum of grades instead of having it divided and then multiplied again; we can thus extract the summing out of its average method and make it available

evap/results/exporters.py

niklasmohrin · 2025-08-03T19:30:53Z

evap/results/exporters.py

        else:
            self.write_cell(questionnaire.public_name, "bold")

+        self.write_cell("", "border_left_right")


this can just be merged with the next line (where the comment is also not correct anymore)

niklasmohrin · 2025-08-03T19:34:38Z

evap/results/exporters.py

        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._get_average_of_average_grade_and_approval_ratio(


together with avg and average_approval_ratio below, it's not really clear what the difference between those four variables is; I think we can should use a clearer name

niklasmohrin · 2025-08-03T19:42:46Z

evap/results/tests/test_exporters.py

        self.assertEqual(sheet.row_values(5)[0], questions[1].text)
        self.assertEqual(sheet.row_values(6)[0], questions[2].text)
+
+    def test_total_average(self):


this does not cover several important details:

evaluations from previous semesters are included

evaluations from other programs and course types are included

only published evaluations are included
a. even if the unpublished evaluations are part of the export

which contributions are used

we are averaging over the averages of several results

are results with not enough voters used

So this test should be extended / more tests should be added. I think it is very easy to mess up some of these details in the future and when that happens, these tests should catch it

Co-authored-by: Niklas Mohrin <dev@niklasmohrin.de>

fidoriel force-pushed the average-grades-in-export branch 5 times, most recently from af9697b to 382680b Compare January 13, 2025 20:13

richardebeling requested changes Jan 19, 2025

View reviewed changes

fidoriel force-pushed the average-grades-in-export branch from dced350 to 6163186 Compare January 27, 2025 18:58

niklasmohrin reviewed Feb 3, 2025

View reviewed changes

richardebeling self-requested a review February 17, 2025 19:11

richardebeling reviewed Feb 24, 2025

View reviewed changes

janno42 requested changes Mar 3, 2025

View reviewed changes

evap/results/exporters.py Outdated Show resolved Hide resolved

fidoriel commented Mar 10, 2025

View reviewed changes

evap/results/exporters.py Outdated Show resolved Hide resolved

Kakadus reviewed Mar 17, 2025

View reviewed changes

evap/staff/views.py Outdated Show resolved Hide resolved

fidoriel requested review from janno42 and richardebeling April 14, 2025 16:48

richardebeling previously approved these changes Apr 14, 2025

View reviewed changes

evap/results/tests/test_exporters.py Outdated Show resolved Hide resolved

richardebeling requested review from Kakadus and niklasmohrin April 14, 2025 18:04

janno42 reviewed Apr 14, 2025

View reviewed changes

evap/results/exporters.py Outdated Show resolved Hide resolved

evap/results/tests/test_exporters.py Outdated Show resolved Hide resolved

Kakadus reviewed May 1, 2025

View reviewed changes

evap/results/exporters.py Outdated Show resolved Hide resolved

fidoriel dismissed richardebeling’s stale review via 86e0602 May 19, 2025 16:13

fidoriel added 9 commits May 19, 2025 18:16

remove private

be4135a

Add average results to exporter

3bed114

add tests for results exporter average

67c3276

adress comments

676bd40

revert to tuple

3bc8814

rename functions

a5eedbf

add style

1bdb5c9

remove useless test

a697d21

rephrase

f618d37

fidoriel added 4 commits May 19, 2025 18:17

fix test

ca1c44f

revert type

d61be22

use hard coded in test

4ae2571

merge tests

eb96a66

fidoriel force-pushed the average-grades-in-export branch from 84a538a to eb96a66 Compare May 19, 2025 16:17

revert changes

8a736d0

janno42 previously approved these changes Jul 14, 2025

View reviewed changes

janno42 requested review from Kakadus and richardebeling July 14, 2025 16:24

niklasmohrin requested changes Aug 3, 2025

View reviewed changes

richardebeling removed their request for review August 4, 2025 15:27

janno42 force-pushed the main branch from 73e348c to 3577b6a Compare August 12, 2025 11:21

janno42 self-requested a review September 1, 2025 14:31

niklasmohrin marked this pull request as draft September 21, 2025 19:19

Update evap/results/exporters.py

b3adec6

Co-authored-by: Niklas Mohrin <dev@niklasmohrin.de>

fidoriel dismissed janno42’s stale review via b3adec6 October 20, 2025 17:22

fidoriel and others added 6 commits October 20, 2025 19:25

Update evap/results/exporters.py

f93ee5b

Co-authored-by: Niklas Mohrin <dev@niklasmohrin.de>

Update evap/results/exporters.py

3163bd6

Co-authored-by: Niklas Mohrin <dev@niklasmohrin.de>

Merge branch 'main' into average-grades-in-export

ee64f6f

Update evap/results/exporters.py

fc91dbc

Co-authored-by: Niklas Mohrin <dev@niklasmohrin.de>

Update evap/results/exporters.py

648cd14

Co-authored-by: Niklas Mohrin <dev@niklasmohrin.de>

make it work with recent changes

a658751

		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):

		def _get_average_grade_and_approval_ratio(
		cls, questionnaire_id: int, question: Question, results: OrderedDict[int, list[QuestionResult]]

Average grades in export #2366

Are you sure you want to change the base?

Average grades in export #2366

Uh oh!

Conversation

fidoriel commented Jan 13, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

niklasmohrin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

janno42 left a comment

Choose a reason for hiding this comment

Uh oh!

niklasmohrin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants