Moodle
  1. Moodle
  2. MDL-36025

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

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor 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
    • Rank:
      44785

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

        Activity

        Hide
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        Matt Petro added a comment -

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

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

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

        Show
        Dan Poltawski added a comment - Thanks Matt, i've integrated this to master, 24, 23 and 22.
        Hide
        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
        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
        Andrew Davis added a comment -

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

        Show
        Andrew Davis added a comment - I tested this yesterday but was unable to pass it due to a problem with tracker. Passing.
        Hide
        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
        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: