Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-13629

Gradebook: Unexpected behavior in selection of lowest grade to drop and resultant category calculation

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: 1.9, 1.9.12
    • Fix Version/s: 2.2.5, 2.3.2
    • Component/s: Gradebook
    • Labels:
    • Database:
      Any
    • Testing Instructions:
      Hide

      log in as admin or teacher.
      delete all gradeable activities (or create a new course)
      create 3 assignments. use "simple direct grading". Set them to have a maximum grade of 5, 10 and 15 respectively.

      You may want to keep the grader report and categories and items "full view" open in 2 tabs.

      Go to the categories and items "full view" screen.
      Click the edit/hand icon for the course grade item.
      Set aggregation to "simple weighted mean of grades".
      Set maximum grade to 100.00
      Go to the grader report and enter a grade of 10 for the assignment out of 10

      On the categories screen, set "drop the lowest" to None.
      Tick "Aggregate only non-empty grades" if it isnt already
      On the grader report, the students course grade should become 100% --> (1.0*10)/10
      1.0 = the student grade, 100%
      the first 10 = the assignment maximum
      the other 10 = the sum of all included activity maximums
      (The student only has a single graded and they got 100% so their course grade is 100%)

      Untick "Aggregate only non-empty grades" for the course
      Reload the grader report.
      Course grade should become 33.33%--> (0.0*5 + 1.0*10 + 0.0*15)/30
      (the other assignments are now included and are counted as 0)

      On the categories screen, set "drop the lowest" to 1.
      Reload the grader report.
      Course grade should become 66.7% (or 66.67 or similar) --> (0.0*5 + 1.0*10)/15
      (the denominator was reduced due to the removal of the assignment out of 15, the most "weighty" assignment)

      Enter a 0 for the assignment out of 15.
      Reload the grader report.
      Course grade should stay at 66.67 --> (0.0*5 + 1.0*10)/15
      (it makes no difference if the assignment grade is 0 or empty)

      Enter a 0 for the assignment out of 5.
      The course grade should stay the same.

      Un-override the assignment out of 15 (it goes back to being empty)
      The course grade should stay the same.

      Un-override the assignment out of 5.
      The course grade should stay the same.

      Un-override the assignment out of 10.
      The course grade should go back to 0.

      Show
      log in as admin or teacher. delete all gradeable activities (or create a new course) create 3 assignments. use "simple direct grading". Set them to have a maximum grade of 5, 10 and 15 respectively. You may want to keep the grader report and categories and items "full view" open in 2 tabs. Go to the categories and items "full view" screen. Click the edit/hand icon for the course grade item. Set aggregation to "simple weighted mean of grades". Set maximum grade to 100.00 Go to the grader report and enter a grade of 10 for the assignment out of 10 On the categories screen, set "drop the lowest" to None. Tick "Aggregate only non-empty grades" if it isnt already On the grader report, the students course grade should become 100% --> (1.0*10)/10 1.0 = the student grade, 100% the first 10 = the assignment maximum the other 10 = the sum of all included activity maximums (The student only has a single graded and they got 100% so their course grade is 100%) Untick "Aggregate only non-empty grades" for the course Reload the grader report. Course grade should become 33.33%--> (0.0*5 + 1.0*10 + 0.0*15)/30 (the other assignments are now included and are counted as 0) On the categories screen, set "drop the lowest" to 1. Reload the grader report. Course grade should become 66.7% (or 66.67 or similar) --> (0.0*5 + 1.0*10)/15 (the denominator was reduced due to the removal of the assignment out of 15, the most "weighty" assignment) Enter a 0 for the assignment out of 15. Reload the grader report. Course grade should stay at 66.67 --> (0.0*5 + 1.0*10)/15 (it makes no difference if the assignment grade is 0 or empty) Enter a 0 for the assignment out of 5. The course grade should stay the same. Un-override the assignment out of 15 (it goes back to being empty) The course grade should stay the same. Un-override the assignment out of 5. The course grade should stay the same. Un-override the assignment out of 10. The course grade should go back to 0.
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull Master Branch:
      MDL-13629_droplow

      Description

      Using simple weighted mean of grade with assignments of varying point values I am not sure what the logic ought to be for selecting the lowest grade. I would assume that it would be the lowest percentage (points earned/points possible) for an assignment and where there are multiple zeros that it would select the one with the highest possible points (i.e. the one that would have the greatest weight).

      I would expect a blank and zero to be treated the same unless the gradebook as been set to ignore blank grades. In my particular case, I did not check the box for the category to Aggregate only non-empty grades. So I believe that an empty grade should be considered equivalent to a zero (at least that is the behavior I am expecting).

      What I noticed was that the category average changed depending on which assignment was blank and which was zero. I would have expected the category value to remain the same. I am attaching the course backup (zip) to facilitate reproduction as well as two screenshots showing the swap between the 0 and blank on the last student with different calculated values for the assignment category.

        Gliffy Diagrams

        1. limits2.patch
          2 kB
          Petr Skoda
        1. blankmin.png
          132 kB
        2. droplowest_a.png
          128 kB
        3. droplowest_b.png
          129 kB
        4. droppingblanks.png
          129 kB

          Issue Links

            Activity

            Hide
            aborrow Anthony Borrow added a comment -

            Just to be clear, in the screenshots - I'm looking at student Ignacio Jones where Assignment 3 and Assignment 4 are swapped between being 0 and blank. If 0 and blank are equivalent (as in this case they should be) then the calculated total for the assignments category should remain 20 and not turn to 50.

            Show
            aborrow Anthony Borrow added a comment - Just to be clear, in the screenshots - I'm looking at student Ignacio Jones where Assignment 3 and Assignment 4 are swapped between being 0 and blank. If 0 and blank are equivalent (as in this case they should be) then the calculated total for the assignments category should remain 20 and not turn to 50.
            Hide
            skodak Petr Skoda added a comment -

            blank grade is treated as minimal grade,
            the limiting rules are applied after converting grade values to percentages

            I think it works fine

            Show
            skodak Petr Skoda added a comment - blank grade is treated as minimal grade, the limiting rules are applied after converting grade values to percentages I think it works fine
            Hide
            aborrow Anthony Borrow added a comment -

            Petr - dropping blanks has Ignacio with two blank scores. Which one should be dropped? I think in the documentation we may need to explain this logic. Do we drop the first one in the list or the one worth the greatest number of points? It looks like it may be the first in the list but I'd have to do more testing to confirm. Peace - Anthony

            Show
            aborrow Anthony Borrow added a comment - Petr - dropping blanks has Ignacio with two blank scores. Which one should be dropped? I think in the documentation we may need to explain this logic. Do we drop the first one in the list or the one worth the greatest number of points? It looks like it may be the first in the list but I'd have to do more testing to confirm. Peace - Anthony
            Hide
            aborrow Anthony Borrow added a comment - - edited

            Petr - In the blankmin.png file, Ignacio's scores do not seem to follow your rule that the blank is dropped. If it were I would expect a higher score (namely 50). Peace - Anthony

            Show
            aborrow Anthony Borrow added a comment - - edited Petr - In the blankmin.png file, Ignacio's scores do not seem to follow your rule that the blank is dropped. If it were I would expect a higher score (namely 50). Peace - Anthony
            Hide
            skodak Petr Skoda added a comment -

            hmm, yes you are right - when dropping lowest/highes the values are ordered by percentage value and if several values are equal the selection is random (which is not a problem in other aggregation types)

            solution could be:
            1/ disable these options in this type of aggregation
            2/ or rewrite the code - add special treatment of weighted means

            Show
            skodak Petr Skoda added a comment - hmm, yes you are right - when dropping lowest/highes the values are ordered by percentage value and if several values are equal the selection is random (which is not a problem in other aggregation types) solution could be: 1/ disable these options in this type of aggregation 2/ or rewrite the code - add special treatment of weighted means
            Hide
            skodak Petr Skoda added a comment -

            please test the attached patch, it might do what you want

            Show
            skodak Petr Skoda added a comment - please test the attached patch, it might do what you want
            Hide
            skodak Petr Skoda added a comment -

            Thinking about this a bit more, I find it problematic to define the "lowest" grade, it can be:
            a/ the lowest entered number (in patch above)
            b/ the lowest percentage value (in cvs: grademin == 0% grademax == 100%)
            c) one of those above affected by item weight too

            Neither above returns easy to guess results imho and it gets even more complicated if normal weighted mean used.

            Show
            skodak Petr Skoda added a comment - Thinking about this a bit more, I find it problematic to define the "lowest" grade, it can be: a/ the lowest entered number (in patch above) b/ the lowest percentage value (in cvs: grademin == 0% grademax == 100%) c) one of those above affected by item weight too Neither above returns easy to guess results imho and it gets even more complicated if normal weighted mean used.
            Hide
            aborrow Anthony Borrow added a comment -

            I'll evaluate and play with the patch and look at the code when I get a chance. I would say that I think I would stick with the CVS behavior (i.e. lowest percentage value). I'm not clear how item weight plays into this but we can deal with this as a fix for 1.9.1.

            Show
            aborrow Anthony Borrow added a comment - I'll evaluate and play with the patch and look at the code when I get a chance. I would say that I think I would stick with the CVS behavior (i.e. lowest percentage value). I'm not clear how item weight plays into this but we can deal with this as a fix for 1.9.1.
            Hide
            salvetore Michael de Raadt added a comment -

            Increasing the priority of this issue as it has now been duplicated twice.

            Show
            salvetore Michael de Raadt added a comment - Increasing the priority of this issue as it has now been duplicated twice.
            Hide
            andyjdavis Andrew Davis added a comment - - edited

            Im going to take a look at this. The bug fixing period at Moodle HQ for Moodle 1.9 has finished. At a minimum I'll need to verify whether or not this is still a problem in more recent versions of Moodle.

            I am attempting to estimate my future work so that I can manage my time better. My current estimate for determing if this is still an issue is 2 hours. Once Ive made that determination I'll have a better idea of how long it will take to get this fixed, integrated and tested.

            Show
            andyjdavis Andrew Davis added a comment - - edited Im going to take a look at this. The bug fixing period at Moodle HQ for Moodle 1.9 has finished. At a minimum I'll need to verify whether or not this is still a problem in more recent versions of Moodle. I am attempting to estimate my future work so that I can manage my time better. My current estimate for determing if this is still an issue is 2 hours. Once Ive made that determination I'll have a better idea of how long it will take to get this fixed, integrated and tested.
            Hide
            aborrow Anthony Borrow added a comment -

            Thanks for verifying this is still an issue in 2.x. Let me know if you need any help testing. Peace - Anthony

            Show
            aborrow Anthony Borrow added a comment - Thanks for verifying this is still an issue in 2.x. Let me know if you need any help testing. Peace - Anthony
            Hide
            andyjdavis Andrew Davis added a comment -

            It appears that this is still an issue. My testing notes are below. Ill eventually use these as inspiration for the testing intructions if you're wondering why they're phrased as they are.

            log in as admin or teacher.
            delete all gradeable activities (or create a new course)
            create 3 assignments. use "simple direct grading". Set them to have a maximum grade of 5, 10 and 15 respectively.

            You may want to keep the grader report and categories and items "full view" open in 2 tabs.

            Go to the categories and items "full view" screen.
            Click the edit/hand icon for the course grade item.
            Set aggregation to "simple weighted mean of grades".
            Set maximum grade to 100.00
            Go to the grader report and enter a grade of 10 for the assignment out of 10

            On the categories screen, set "drop the lowest" to None.
            Tick "Aggregate only non-empty grades" if it isnt already
            On the grader report, the students course grade should become 100% --> (1.0*10)/10
            1.0 = the student grade, 100%
            the first 10 = the assignment maximum
            the other 10 = the sum of all included activity maximums
            (The student only has a single graded and they got 100% so their course grade is 100%)

            Untick "Aggregate only non-empty grades" for the course
            Reload the grader report.
            Course grade should become 33.33%--> (0.0*5 + 1.0*10 + 0.0*15)/30
            (the other assignments are now included and are counted as 0.

            On the categories screen, set "drop the lowest" to 1.
            Reload the grader report.
            Course grade should become 66.7% --> (0.0*5 + 1.0*10)/15
            (the denominator was reduced due to the removal of the assignment out of 15, the most "weighty" assignment)

            Enter a 0 for the assignment out of 15.
            Reload the grader report.
            Course grade becomes 40.0% --> (1.0*10 + 0.0*15)/25
            (now the assignment out of 5 is being removed)

            Enter a 0 for the assignment out of 5.
            Un-override the assignment out of 15 (it goes back to being empty)
            Reload the grader report.
            Course grade becomes 66.67.0% --> (0.0*5 + 1.0*10)/15
            (now the assignment out of 15 is again being removed. It appears that empty grades are dropped in preference to 0s. I'm not sure this is correct. It seems like we should be considering 0s and empties equal, assuming the minimum grade is 0, and thus still be dropping the most "weighty" assignment)

            Show
            andyjdavis Andrew Davis added a comment - It appears that this is still an issue. My testing notes are below. Ill eventually use these as inspiration for the testing intructions if you're wondering why they're phrased as they are. log in as admin or teacher. delete all gradeable activities (or create a new course) create 3 assignments. use "simple direct grading". Set them to have a maximum grade of 5, 10 and 15 respectively. You may want to keep the grader report and categories and items "full view" open in 2 tabs. Go to the categories and items "full view" screen. Click the edit/hand icon for the course grade item. Set aggregation to "simple weighted mean of grades". Set maximum grade to 100.00 Go to the grader report and enter a grade of 10 for the assignment out of 10 On the categories screen, set "drop the lowest" to None. Tick "Aggregate only non-empty grades" if it isnt already On the grader report, the students course grade should become 100% --> (1.0*10)/10 1.0 = the student grade, 100% the first 10 = the assignment maximum the other 10 = the sum of all included activity maximums (The student only has a single graded and they got 100% so their course grade is 100%) Untick "Aggregate only non-empty grades" for the course Reload the grader report. Course grade should become 33.33%--> (0.0*5 + 1.0*10 + 0.0*15)/30 (the other assignments are now included and are counted as 0. On the categories screen, set "drop the lowest" to 1. Reload the grader report. Course grade should become 66.7% --> (0.0*5 + 1.0*10)/15 (the denominator was reduced due to the removal of the assignment out of 15, the most "weighty" assignment) Enter a 0 for the assignment out of 15. Reload the grader report. Course grade becomes 40.0% --> (1.0*10 + 0.0*15)/25 (now the assignment out of 5 is being removed) Enter a 0 for the assignment out of 5. Un-override the assignment out of 15 (it goes back to being empty) Reload the grader report. Course grade becomes 66.67.0% --> (0.0*5 + 1.0*10)/15 (now the assignment out of 15 is again being removed. It appears that empty grades are dropped in preference to 0s. I'm not sure this is correct. It seems like we should be considering 0s and empties equal, assuming the minimum grade is 0, and thus still be dropping the most "weighty" assignment)
            Hide
            andyjdavis Andrew Davis added a comment -

            In lib/grade/tests/grade_category_test.php is a test method sub_test_grade_category_apply_limit_rules() that tests grade category "droplow" and "keephigh". I'm now attempting to expand the test set to include the reproduction of this bug. If I can manage that I can be confident of any subsequent fix and can ensure this bug doesn't come up again in future.

            Show
            andyjdavis Andrew Davis added a comment - In lib/grade/tests/grade_category_test.php is a test method sub_test_grade_category_apply_limit_rules() that tests grade category "droplow" and "keephigh". I'm now attempting to expand the test set to include the reproduction of this bug. If I can manage that I can be confident of any subsequent fix and can ensure this bug doesn't come up again in future.
            Hide
            andyjdavis Andrew Davis added a comment -

            Im adding a git branch that contains enhanced unit tests. I've added a new test that reproduces the problem. There are two grade items with a 0 grade. One has a maxgrade of 100, the other has a maxgrade of 110. Currently the grade item with a maxgrade of 100 is being excluded by droplow which is incorrect. This unit test will fail until apply_limit_rules() is fixed which is my next task.

            Show
            andyjdavis Andrew Davis added a comment - Im adding a git branch that contains enhanced unit tests. I've added a new test that reproduces the problem. There are two grade items with a 0 grade. One has a maxgrade of 100, the other has a maxgrade of 110. Currently the grade item with a maxgrade of 100 is being excluded by droplow which is incorrect. This unit test will fail until apply_limit_rules() is fixed which is my next task.
            Hide
            andyjdavis Andrew Davis added a comment -

            Ok, I think this is all done. I've made apply_limit_rules() a bit smarter about which grade items it drops when droplow is used. Putting this up for peer review.

            Show
            andyjdavis Andrew Davis added a comment - Ok, I think this is all done. I've made apply_limit_rules() a bit smarter about which grade items it drops when droplow is used. Putting this up for peer review.
            Hide
            dmonllao David Monllaó added a comment -

            Hi Andrew,

            I see that grade_values is ordered by the value, but the order of the elements with the same value will not be always the same, so checking only the next item grademax could not be enough to be sure that we always skip the one with the same value and the highest grademax.

            I tested your scenario with 3 different users on the course, the dropped items differs for every student and the course final grade also differs.

            Thanks

            Show
            dmonllao David Monllaó added a comment - Hi Andrew, I see that grade_values is ordered by the value, but the order of the elements with the same value will not be always the same, so checking only the next item grademax could not be enough to be sure that we always skip the one with the same value and the highest grademax. I tested your scenario with 3 different users on the course, the dropped items differs for every student and the course final grade also differs. Thanks
            Hide
            andyjdavis Andrew Davis added a comment - - edited

            Thanks David. Maybe I'm going about this the wrong way but its a deceptively complex little problem. I think I have it now. I'll take another look in the morning.

            Show
            andyjdavis Andrew Davis added a comment - - edited Thanks David. Maybe I'm going about this the wrong way but its a deceptively complex little problem. I think I have it now. I'll take another look in the morning.
            Hide
            salvetore Michael de Raadt added a comment -

            Carrying over to the next sprint.

            Show
            salvetore Michael de Raadt added a comment - Carrying over to the next sprint.
            Hide
            andyjdavis Andrew Davis added a comment -

            Putting this up for peer review.

            Show
            andyjdavis Andrew Davis added a comment - Putting this up for peer review.
            Hide
            dmonllao David Monllaó added a comment -

            Sorry Andrew, it looks ok with the extra while, but testing the same scenario I've noticed the same issue, it seems that is here https://github.com/andyjdavis/moodle/compare/master...MDL-13629_droplow#L0R915, the assigned value is a float and the value of the empty grades item comes as an integer

            Show
            dmonllao David Monllaó added a comment - Sorry Andrew, it looks ok with the extra while, but testing the same scenario I've noticed the same issue, it seems that is here https://github.com/andyjdavis/moodle/compare/master...MDL-13629_droplow#L0R915 , the assigned value is a float and the value of the empty grades item comes as an integer
            Hide
            andyjdavis Andrew Davis added a comment -

            Added testing instructions and an updated fix.

            Show
            andyjdavis Andrew Davis added a comment - Added testing instructions and an updated fix.
            Hide
            dmonllao David Monllaó added a comment -

            It looks good and passes the test, thanks Andrew. Finishing peer review

            Show
            dmonllao David Monllaó added a comment - It looks good and passes the test, thanks Andrew. Finishing peer review
            Hide
            andyjdavis Andrew Davis added a comment -

            Created the various branches. Putting this up for integration.

            Show
            andyjdavis Andrew Davis added a comment - Created the various branches. Putting this up for integration.
            Hide
            poltawski Dan Poltawski added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            poltawski Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            poltawski Dan Poltawski added a comment -

            Hi Andrew,

            Taken some time trying to get my head around all this, its quite complex logic going on within loops. Great to see unit tests for this.

            The thing i'm finding difficult to understand is the purpose of the second while loop:
            while ($originalindex+$i < count($grade_keys))

            I just don't really know whats going on in there, so I was wondering if you could add a comment to the code to explain whats going on there?

            Show
            poltawski Dan Poltawski added a comment - Hi Andrew, Taken some time trying to get my head around all this, its quite complex logic going on within loops. Great to see unit tests for this. The thing i'm finding difficult to understand is the purpose of the second while loop: while ($originalindex+$i < count($grade_keys)) I just don't really know whats going on in there, so I was wondering if you could add a comment to the code to explain whats going on there?
            Hide
            cibot CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            andyjdavis Andrew Davis added a comment -

            I've added an explanatory comment that hopefully makes it more clear.

            The goal is to drop n grade items with the lowest grade values (the grade the student has obtained). Extra credit grade items are never dropped. Where multiple grade items have the same grade value, for example 0, we need to drop the grade items with the highest grademax.

            Show
            andyjdavis Andrew Davis added a comment - I've added an explanatory comment that hopefully makes it more clear. The goal is to drop n grade items with the lowest grade values (the grade the student has obtained). Extra credit grade items are never dropped. Where multiple grade items have the same grade value, for example 0, we need to drop the grade items with the highest grademax.
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks Andrew. I'll try to integrate and test this myself today because I don't think we have the capacity for anyone else to take this testing on.

            Show
            poltawski Dan Poltawski added a comment - Thanks Andrew. I'll try to integrate and test this myself today because I don't think we have the capacity for anyone else to take this testing on.
            Hide
            poltawski Dan Poltawski added a comment -

            Integrated, thanks Andrew.

            I've tested 22 and master during integration review. Going to test on 23 now.

            Show
            poltawski Dan Poltawski added a comment - Integrated, thanks Andrew. I've tested 22 and master during integration review. Going to test on 23 now.
            Hide
            poltawski Dan Poltawski added a comment -

            Looks good on 2.3 too. Test passed, thanks Andrew.

            Show
            poltawski Dan Poltawski added a comment - Looks good on 2.3 too. Test passed, thanks Andrew.
            Hide
            nebgor Aparup Banerjee added a comment -

            yay, it works!

            This issue has been put through rigorous processes and finally swam upstream along with some 65 others this week.

            Thank you all for taking the time to get us here.

            cheers!

            Show
            nebgor Aparup Banerjee added a comment - yay, it works! This issue has been put through rigorous processes and finally swam upstream along with some 65 others this week. Thank you all for taking the time to get us here. cheers!

              People

              • Votes:
                0 Vote for this issue
                Watchers:
                2 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  10/Sep/12