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

Error with dataset value "-0" in calculated question type

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.2, 2.4
    • Fix Version/s: 2.2.7, 2.3.4, 2.4.1
    • Component/s: Quiz
    • Labels:
    • Testing Instructions:
      Hide

      1. Create a calculated question with a correct answer like

      {a}

      -

      {b}

      .

      2. Set the corrected answer to be calculated with a set number of significant figures.

      3. Create a dataset involving the number -0 for b. (In IEEE floating point, -0 is different from 0.)

      4. Preview the question. Make sure there is not an error message about a formula error.

      Show
      1. Create a calculated question with a correct answer like {a} - {b} . 2. Set the corrected answer to be calculated with a set number of significant figures. 3. Create a dataset involving the number -0 for b. (In IEEE floating point, -0 is different from 0.) 4. Preview the question. Make sure there is not an error message about a formula error.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-36025-negative-zero

      Description

      The processing of "-0" as a dataset value is incorrect. I'm not sure the circumstances that create "-0", but moodle must be creating it as we have lots on our site. It's also possible to enter "-0" manually as a dataset value.

      The bug is in evaluating a formula with the "-0". In qtype_calculate - >substitute_variables(), it won't use parentheses around "-0" since "-0" >= 0. For example. -

      {A}

      becomes in --0. This results in a PHP error being printed.

      For format "Significant figures", this actually results in an infinite loop since there is no check that $answer is numeric before executing:

      qtype_calculated_calculate_answer():

              $p10 = 0;
              while ($answer < 1) {
                  --$p10;
                  $answer *= 10;
              }

      Suggested fixes:

      1. In qtype_calculate->substitute_variables(), use parens for $val <= 0
      2. In qtype_calculate->substitute_variables_and_eval(), use a different variable than $str in the eval(). Otherwise, if the eval() fails, the function will return the original formula. This triggers the infinite loop above.
      3. Check for and reject "--" (and "++") in qtype_calculated_find_formula_errors().

        Gliffy Diagrams

          Activity

          Hide
          mpetrowi Matt Petro added a comment -

          Also
          4. Add a check that $answer is numeric before running the above loop. Eval() seems like something of a wild card to me, and I wonder if we can really guarantee it'll never return anything weird? An infinite loop makes for a bad user experience

          Show
          mpetrowi Matt Petro added a comment - Also 4. Add a check that $answer is numeric before running the above loop. Eval() seems like something of a wild card to me, and I wonder if we can really guarantee it'll never return anything weird? An infinite loop makes for a bad user experience
          Hide
          timhunt Tim Hunt added a comment -

          You are right, this is odd.

          I was hoping that a judicious cast to (float) would fix the -0 issue, but it doesn't:

          echo '<pre>';
          $a = '-0';
          echo (float) $a, "\n";
          $a = (float) $a;
          echo ">=: ", $a >= 0, "\n";
          echo ">: ", $a > 0, "\n";
          echo '</pre>';

          outputs -0, true, false.

          So, I think I prefer your option 1.

          For the infinite loop, we need to handle 0 as a special case.

          Are you intending to do a patch for this, or do I need to?

          Show
          timhunt Tim Hunt added a comment - You are right, this is odd. I was hoping that a judicious cast to (float) would fix the -0 issue, but it doesn't: echo '<pre>'; $a = '-0'; echo (float) $a, "\n"; $a = (float) $a; echo ">=: ", $a >= 0, "\n"; echo ">: ", $a > 0, "\n"; echo '</pre>'; outputs -0, true, false. So, I think I prefer your option 1. For the infinite loop, we need to handle 0 as a special case. Are you intending to do a patch for this, or do I need to?
          Hide
          mpetrowi Matt Petro added a comment -

          I didn't realize this before: php is correct that float(-0) == float(0) in IEEE floating point. See http://en.wikipedia.org/wiki/Negative_zero#Comparisons

          Sure, I'll make a patch.

          Show
          mpetrowi Matt Petro added a comment - I didn't realize this before: php is correct that float(-0) == float(0) in IEEE floating point. See http://en.wikipedia.org/wiki/Negative_zero#Comparisons Sure, I'll make a patch.
          Hide
          mpetrowi Matt Petro added a comment -

          Here's a fix for 1,2, and 4.

          For 4. I'm just checking if the result is numeric or not. (i.e. if it's a numeric answer or an error message, or null.) This will prevent the infinite loop if for some reason the eval() fails.

          The error handling in this qtype could use a lot of work!

          Show
          mpetrowi Matt Petro added a comment - Here's a fix for 1,2, and 4. For 4. I'm just checking if the result is numeric or not. (i.e. if it's a numeric answer or an error message, or null.) This will prevent the infinite loop if for some reason the eval() fails. The error handling in this qtype could use a lot of work!
          Hide
          timhunt Tim Hunt added a comment -

          substitute_variables_and_eval can return '*'. In that case, is your change in qtype_calculated_calculate_answer the right thing? (I can't work it out now.) This qtype could also use a lot more PHPdoc comments to explain what these functions are supposed to do!

          These changes feel to me like the sort of thing that should have unit tests. It would also help document the expected behaviour of these functions. Do you have time to add some?

          Thanks. P.S. I do remember I owe you a code-review of MDL-35717.

          Show
          timhunt Tim Hunt added a comment - substitute_variables_and_eval can return '*'. In that case, is your change in qtype_calculated_calculate_answer the right thing? (I can't work it out now.) This qtype could also use a lot more PHPdoc comments to explain what these functions are supposed to do! These changes feel to me like the sort of thing that should have unit tests. It would also help document the expected behaviour of these functions. Do you have time to add some? Thanks. P.S. I do remember I owe you a code-review of MDL-35717 .
          Hide
          mpetrowi Matt Petro added a comment -

          What happens with * is that qtype_calculated_calculate_answer() returns NaN, but then comment_on_datasetitems() turns around and overwrites the answer with *. None of the code below the is_numeric() in qtype_calculated_calculate_answer() makes any sense for $answer = * (or anything non-numeric), which is why I just have it return NaN. I chose NaN because if the formula has an error then at least NaN means it won't match any student response.

          I think this fixes the immediate problem we're seeing. I've got other projects right now that are taking my time, so I can't really get into it beyond this fix.

          (Oh, and the other caller of qtype_calculated_calculate_answer() is in multichoice_comment_on_datasetitems(), but that's apparently never called. Maybe it's obsoleted by the calculatedmulti qtype?)

          Show
          mpetrowi Matt Petro added a comment - What happens with * is that qtype_calculated_calculate_answer() returns NaN, but then comment_on_datasetitems() turns around and overwrites the answer with *. None of the code below the is_numeric() in qtype_calculated_calculate_answer() makes any sense for $answer = * (or anything non-numeric), which is why I just have it return NaN. I chose NaN because if the formula has an error then at least NaN means it won't match any student response. I think this fixes the immediate problem we're seeing. I've got other projects right now that are taking my time, so I can't really get into it beyond this fix. (Oh, and the other caller of qtype_calculated_calculate_answer() is in multichoice_comment_on_datasetitems(), but that's apparently never called. Maybe it's obsoleted by the calculatedmulti qtype?)
          Hide
          timhunt Tim Hunt added a comment -

          Right, I understand now. Thanks for explaining. We can look at cleaning up the obsolete code, and adding more unit tests later.

          Can you re-base and cherry-pick to stable branches, then I will submit this for integration. Thanks.

          Show
          timhunt Tim Hunt added a comment - Right, I understand now. Thanks for explaining. We can look at cleaning up the obsolete code, and adding more unit tests later. Can you re-base and cherry-pick to stable branches, then I will submit this for integration. Thanks.
          Hide
          mpetrowi Matt Petro added a comment -

          Hopefully I did the re-base and cherry-picking right. Does anything else need to be done for integration?

          Show
          mpetrowi Matt Petro added a comment - Hopefully I did the re-base and cherry-picking right. Does anything else need to be done for integration?
          Hide
          timhunt Tim Hunt added a comment -

          2.1 is not supported any more (apart from security fixes) so your only mistake was to cherry pick to that branch.

          I hope the testing instructions I wrote are OK.

          Show
          timhunt Tim Hunt added a comment - 2.1 is not supported any more (apart from security fixes) so your only mistake was to cherry pick to that branch. I hope the testing instructions I wrote are OK.
          Hide
          mpetrowi Matt Petro added a comment -

          Ah, and I forgot to write testing instructions. Thanks for your help on this, Tim.

          Show
          mpetrowi Matt Petro added a comment - Ah, and I forgot to write testing instructions. Thanks for your help on this, Tim.
          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 -

          I've not seen NAN used before so I checked it was a php predefined constant: http://php.net/manual/en/math.constants.php

          Show
          poltawski Dan Poltawski added a comment - I've not seen NAN used before so I checked it was a php predefined constant: http://php.net/manual/en/math.constants.php
          Hide
          poltawski Dan Poltawski added a comment -

          Thanks Matt, i've integrated this to master, 24, 23 and 22.

          Show
          poltawski Dan Poltawski added a comment - Thanks Matt, i've integrated this to master, 24, 23 and 22.
          Hide
          timhunt Tim Hunt added a comment -

          Yes, I had to check that too. Perhaps I should have added a comment at that point? Oh well.

          Show
          timhunt Tim Hunt added a comment - Yes, I had to check that too. Perhaps I should have added a comment at that point? Oh well.
          Hide
          andyjdavis Andrew Davis added a comment -

          I tested this yesterday but was unable to pass it due to a problem with tracker. Passing.

          Show
          andyjdavis Andrew Davis added a comment - I tested this yesterday but was unable to pass it due to a problem with tracker. Passing.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Many thanks for your effort, the whole Moodle Community will be enjoying your great solutions starting now!

          Closing, ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Many thanks for your effort, the whole Moodle Community will be enjoying your great solutions starting now! Closing, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                14/Jan/13