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

Cancelled Tutorials in Average Attendance Calculation #10136

Open
demetKayabas opened this issue Jan 13, 2025 · 9 comments
Open

Cancelled Tutorials in Average Attendance Calculation #10136

demetKayabas opened this issue Jan 13, 2025 · 9 comments
Labels
bug good first issue tutorialgroup Pull requests that affect the corresponding module

Comments

@demetKayabas
Copy link

demetKayabas commented Jan 13, 2025

Describe the bug

Currently, Artemis includes cancelled tutorials in the average attendance calculation for a specific tutorial group. This can lead to misleading statistics, particularly for groups frequently affected by public holidays. The inclusion of cancelled sessions skews the average attendance, making it less reflective of the actual participation in attended sessions.

To Reproduce

  1. Go to Course Management -> Create a tutorial group.
  2. Mark one or more tutorials as cancelled.
  3. Observe the average attendance of the tutorial group.

Expected behavior

Cancelled tutorials should not be factored into the average attendance calculation. Only tutorials that have taken place and have recorded attendance should contribute to the average.

Screenshots

Artemis Attendance List

Which version of Artemis are you seeing the problem on?

7.8.3

What browsers are you seeing the problem on?

Other (specify in "Additional context")

Additional context

Browser: Arc

Relevant log output

No response

@github-actions github-actions bot added exercise Pull requests that affect the corresponding module lecture Pull requests that affect the corresponding module labels Jan 13, 2025
@krusche
Copy link
Member

krusche commented Jan 13, 2025

I think cancelled sessions should be canceled in Artemis. I thought we have a feature for it to exclude sessions on public holidays

@krusche
Copy link
Member

krusche commented Jan 13, 2025

You are not supposed to enter 0 then

@demetKayabas
Copy link
Author

You are not supposed to enter 0 then

I marked them as cancelled and did not enter any number for those. However, Artemis marks them with 0 and includes in the average.

@pvlov
Copy link

pvlov commented Jan 13, 2025

From a Quick Look at the source code, the following code in TutorialGroupService seems to set the average attendance?

/**
     * Sets the averageAttendance transient field of the given tutorial group
     * <p>
     * Calculation:
     * <ul>
     * <li>Get set of the last three completed sessions (or less than three if not more available)</li>
     * <li>Remove sessions without attendance data (null) from the set</li>
     * <li>If set is empty, set attendance average of tutorial group to null (meaning could not be determined)</li>
     * <li>If set is non empty, set the attendance average of the tutorial group to the arithmetic mean (rounded to integer)</li>
     * </ul>
     *
     * @param tutorialGroup the tutorial group to set the averageAttendance for
     */
    private void setAverageAttendance(TutorialGroup tutorialGroup) {
        Collection<TutorialGroupSession> sessions;
        if (getPersistenceUtil().isLoaded(tutorialGroup, "tutorialGroupSessions") && tutorialGroup.getTutorialGroupSessions() != null) {
            sessions = tutorialGroup.getTutorialGroupSessions();
        }
        else {
            sessions = tutorialGroupSessionRepository.findAllByTutorialGroupId(tutorialGroup.getId());
        }
        sessions.stream()
                .filter(tutorialGroupSession -> TutorialGroupSessionStatus.ACTIVE.equals(tutorialGroupSession.getStatus())
                        && tutorialGroupSession.getEnd().isBefore(ZonedDateTime.now()))
                .sorted(Comparator.comparing(TutorialGroupSession::getStart).reversed()).limit(3)
                .map(tutorialGroupSession -> Optional.ofNullable(tutorialGroupSession.getAttendanceCount())).flatMap(Optional::stream).mapToInt(attendance -> attendance).average()
                .ifPresentOrElse(value -> tutorialGroup.setAverageAttendance((int) Math.round(value)), () -> tutorialGroup.setAverageAttendance(null));
    }

This seems to already take cancelled Sessions into account.

@pvlov
Copy link

pvlov commented Jan 13, 2025

However, from the Screenshot one would expect the average attendance to be 25 instead of 23.

Session 1: attendance 25
Session 2: Cancelled
Session 3: Cancelled
=> 25 / 1 => 25

@pvlov
Copy link

pvlov commented Jan 13, 2025

I think I found the bug: Stream::limit does not actually cut off 3 elements from the Stream but rather stops after processing 3 elements. In this case leading to processing the last three active sessions. This results in the observed behavior:

(25 + 9 + 34) / 3 = 22,667. This then probably gets rounded up because we are converting it to int which leads to 23.

While there is probably a cleaner solution, something along the lines of this should do the trick:

// code...
sessions.stream()
            .filter(tutorialGroupSession -> tutorialGroupSession.getEnd().isBefore(ZonedDateTime.now()))
            .sorted(Comparator.comparing(TutorialGroupSession::getStart).reversed())
            .limit(3)
            .toList()
            .stream()
            .filter(tutorialGroupSession -> TutorialGroupSessionStatus.ACTIVE.equals(tutorialGroupSession.getStatus()))
            .map(tutorialGroupSession -> Optional.ofNullable(tutorialGroupSession.getAttendanceCount())).flatMap(Optional::stream).mapToInt(attendance -> attendance).average()
            .ifPresentOrElse(value -> tutorialGroup.setAverageAttendance((int) Math.round(value)), () -> tutorialGroup.setAverageAttendance(null));

@demetKayabas
Copy link
Author

I think I found the bug: Stream::limit does not actually cut off 3 elements from the Stream but rather stops after processing 3 elements. In this case leading to processing the last three active sessions. This results in the observed behavior:

(25 + 9 + 34) / 3 = 22,667. This then probably gets rounded up because we are converting it to int which leads to 23.

This corresponds to the expected behaviour. Thanks for clarifying.

@pvlov
Copy link

pvlov commented Jan 13, 2025

This should still be a bug tho. The PR should be easy, feel free to contribute 😄

@pvlov pvlov added good first issue tutorialgroup Pull requests that affect the corresponding module and removed exercise Pull requests that affect the corresponding module lecture Pull requests that affect the corresponding module labels Jan 13, 2025
@Zeocode
Copy link

Zeocode commented Jan 21, 2025

I would like to. This will be my first contribution. I have already set up the project in local with some tweaking. I need help on a couple of things like how to get an account to login. Thanks in advance!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug good first issue tutorialgroup Pull requests that affect the corresponding module
Projects
None yet
Development

No branches or pull requests

4 participants