Moodle
  1. Moodle
  2. MDL-31680

Calculated correct response does not apply the format set (significant figure or decimals)

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.4, 2.2.1
    • Fix Version/s: 2.6
    • Component/s: Questions
    • Testing Instructions:
      Hide

      Create a calculated question using a simple equation like

      {x}+0.012345 set the response to 2 decimals
      generate a {x}

      value between 10 and 90 (no decimal) say 45.
      The good response should 45.01 and the same value should show when looking at the wild card value.
      Attempt the saved question, the fill correct response should display the same value and on submit and finish the correct answer should also agree.

      Repeat the tets with 1 decimals. The correct answer should be 45.0 .

      Add a unit say cm and the numerical value displayed should remain the same.

      Add another unit say m multiplier 0.01 and try with the two options for unit (multiple choice or drop-down list)
      The decimal should remain the same either when filling the corect response or in the correct response at the end.
      Do some test with calculated simple.

      The set the option to 4 significant figures. The values displayed should be
      the same as when setting to significant figures.
      Do the tests ( looking at the values for the wild cards values as when doing the previews.
      The https://tracker.moodle.org/secure/attachment/31662/questions-CHI0310_100-test%20MDL-31680-20130313-1325.xml
      contains many questions that illustrate various cases most of them are reproduced in the php test unit.

      The https://tracker.moodle.org/secure/attachment/32775/questions-T1-IMPORT%20MDL-31680-20130515-2347.xml contains three questions with long answers that illustrate their rendering when editing at the datasetitems_form.php for calculated and calculatedmulti
      you should see something like for

      • datasetitems_form.php for calculatedmulti
          
        La componente es ... La componente es {=(123456786.*pow(123456781.1,2)+1234567890.4*123456786.+123456781.1*1234567890.4)/(pow(123456781.1,2)+pow(123456786.,2))}
        

        The left part is truncated ... but the right one not

      • datasetitems_form.php for calculated or in edit_calculated_simple_form.php
        Set 1  {x}+{x}+{x}+{x}+{...
        123456789+123456789+123456789+123456789+123456789+1234567... = 9.88e8
        

      Additional validation has been added when entering new param values:
      error will appear on

      • adding a , in a number : 1,2 gives The , cannot be used, use . as in 0.013 or 1.3e-2
      • adding a non valid number : E-12cm gives Wild card value is not a valid number
      • adding an hexadecimal : 0X13D gives Dataset hexadecimal format value 0X13D is not allowed

      An additional test is done if the formula give a NAN result which result is a non valid number.

      These validation will appears if after entering one of these error in a wildcard value
      you click on the Add item or on save button for

      • datasetitems_form.php for calculated and calculatedmulti
      • and in edit_calculated_simple_form.php.
      Show
      Create a calculated question using a simple equation like {x}+0.012345 set the response to 2 decimals generate a {x} value between 10 and 90 (no decimal) say 45. The good response should 45.01 and the same value should show when looking at the wild card value. Attempt the saved question, the fill correct response should display the same value and on submit and finish the correct answer should also agree. Repeat the tets with 1 decimals. The correct answer should be 45.0 . Add a unit say cm and the numerical value displayed should remain the same. Add another unit say m multiplier 0.01 and try with the two options for unit (multiple choice or drop-down list) The decimal should remain the same either when filling the corect response or in the correct response at the end. Do some test with calculated simple. The set the option to 4 significant figures. The values displayed should be the same as when setting to significant figures. Do the tests ( looking at the values for the wild cards values as when doing the previews. The https://tracker.moodle.org/secure/attachment/31662/questions-CHI0310_100-test%20MDL-31680-20130313-1325.xml contains many questions that illustrate various cases most of them are reproduced in the php test unit. The https://tracker.moodle.org/secure/attachment/32775/questions-T1-IMPORT%20MDL-31680-20130515-2347.xml contains three questions with long answers that illustrate their rendering when editing at the datasetitems_form.php for calculated and calculatedmulti you should see something like for datasetitems_form.php for calculatedmulti La componente es ... La componente es {=(123456786.*pow(123456781.1,2)+1234567890.4*123456786.+123456781.1*1234567890.4)/(pow(123456781.1,2)+pow(123456786.,2))} The left part is truncated ... but the right one not datasetitems_form.php for calculated or in edit_calculated_simple_form.php Set 1 {x}+{x}+{x}+{x}+{... 123456789+123456789+123456789+123456789+123456789+1234567... = 9.88e8 Additional validation has been added when entering new param values: error will appear on adding a , in a number : 1,2 gives The , cannot be used, use . as in 0.013 or 1.3e-2 adding a non valid number : E-12cm gives Wild card value is not a valid number adding an hexadecimal : 0X13D gives Dataset hexadecimal format value 0X13D is not allowed An additional test is done if the formula give a NAN result which result is a non valid number. These validation will appears if after entering one of these error in a wildcard value you click on the Add item or on save button for datasetitems_form.php for calculated and calculatedmulti and in edit_calculated_simple_form.php.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      38257

      Description

      The correct response displayed i.e. 2,78023456 is not set as defined when setting the correct answer.
      see http://moodle.org/mod/forum/discuss.php?d=188637

        Issue Links

          Activity

          Hide
          Pierre Pichet added a comment -

          The renderer use by calculated is the numerical one

          class qtype_calculated_renderer extends qtype_numerical_renderer {
          }
              public function correct_response(question_attempt $qa) {
                  $question = $qa->get_question();
                  $answer = $question->get_correct_answer();
                  if (!$answer) {
                      return '';
                  }
          
                  $response = str_replace('.', $question->ap->get_point(), $answer->answer);
                  if ($question->unitdisplay != qtype_numerical::UNITNONE) {
                      $response = $question->ap->add_unit($response);
                  }
          
                  return get_string('correctansweris', 'qtype_shortanswer', $response);
              }
          

          The answer numerical value shoud be displayed using the calculated/question.php

          class qtype_calculated_variable_substituter {
              /**
               * Display a float properly formatted with a certain number of decimal places.
               * @param number $x the number to format
               * @param int $length restrict to this many decimal places or significant
               *      figures. If null, the number is not rounded.
               * @param int format 1 => decimalformat, 2 => significantfigures.
               * @return string formtted number.
               */
              public function format_float($x, $length = null, $format = null) {
                  if (!is_null($length) && !is_null($format)) {
                      if ($format == 1) {
                          // Decimal places.
                          $x = sprintf('%.' . $length . 'F', $x);
                      } else if ($format == 2) {
                          // Significant figures.
                          $x = sprintf('%.' . $length . 'g', $x);
                      }
                  }
                  return str_replace('.', $this->decimalpoint, $x);
              }
          

          This should give the same results as the "old"
          calculated/questiontype.php

          function qtype_calculated_calculate_answer($formula, $individualdata,
              $tolerance, $tolerancetype, $answerlength, $answerformat = '1', $unit = '') {
          ...
          

          However the "new"code use standard C printf() functions, however qtype_calculated_calculate_answer does not seem to mimic these C functions exactly.
          There is also some recent modifications of the "old" calculated function.
          I am not sure which is best...
          Tim,
          your advice...

          Show
          Pierre Pichet added a comment - The renderer use by calculated is the numerical one class qtype_calculated_renderer extends qtype_numerical_renderer { } public function correct_response(question_attempt $qa) { $question = $qa->get_question(); $answer = $question->get_correct_answer(); if (!$answer) { return ''; } $response = str_replace('.', $question->ap->get_point(), $answer->answer); if ($question->unitdisplay != qtype_numerical::UNITNONE) { $response = $question->ap->add_unit($response); } return get_string('correctansweris', 'qtype_shortanswer', $response); } The answer numerical value shoud be displayed using the calculated/question.php class qtype_calculated_variable_substituter { /** * Display a float properly formatted with a certain number of decimal places. * @param number $x the number to format * @param int $length restrict to this many decimal places or significant * figures. If null, the number is not rounded. * @param int format 1 => decimalformat, 2 => significantfigures. * @return string formtted number. */ public function format_float($x, $length = null, $format = null) { if (!is_null($length) && !is_null($format)) { if ($format == 1) { // Decimal places. $x = sprintf('%.' . $length . 'F', $x); } else if ($format == 2) { // Significant figures. $x = sprintf('%.' . $length . 'g', $x); } } return str_replace('.', $this->decimalpoint, $x); } This should give the same results as the "old" calculated/questiontype.php function qtype_calculated_calculate_answer($formula, $individualdata, $tolerance, $tolerancetype, $answerlength, $answerformat = '1', $unit = '') { ... However the "new"code use standard C printf() functions, however qtype_calculated_calculate_answer does not seem to mimic these C functions exactly. There is also some recent modifications of the "old" calculated function. I am not sure which is best... Tim, your advice...
          Hide
          Pierre Pichet added a comment -

          There is a working solution by adding public function correct_response(question_attempt $qa) {
          in calculated renderer and using the format_float from calculated/question.php .

          More testing and probably adding supplementary tests in simpletest.

          Show
          Pierre Pichet added a comment - There is a working solution by adding public function correct_response(question_attempt $qa) { in calculated renderer and using the format_float from calculated/question.php . More testing and probably adding supplementary tests in simpletest.
          Hide
          Pierre Pichet added a comment -

          ON testing I discover that datasetitems_form.php does not validate modifications
          done to good response parameters ( i.e. significant figures) against datasets values.

          Show
          Pierre Pichet added a comment - ON testing I discover that datasetitems_form.php does not validate modifications done to good response parameters ( i.e. significant figures) against datasets values.
          Hide
          Alan Arnold added a comment -

          Any progess on this Pierre? I'm not sure of the implications of the datasetitems_form.php issue that you have just discovered. Would it help if I asked our Moodle partner (NetSpot) to work with you if it might expedite resolution?

          Show
          Alan Arnold added a comment - Any progess on this Pierre? I'm not sure of the implications of the datasetitems_form.php issue that you have just discovered. Would it help if I asked our Moodle partner (NetSpot) to work with you if it might expedite resolution?
          Hide
          Pierre Pichet added a comment -

          Alan,
          To keep it simple for you
          add the function correct_response in calculated/renderer.php

          class qtype_calculated_renderer extends qtype_numerical_renderer {
              public function correct_response(question_attempt $qa) {
                  $question = $qa->get_question();
                  $answer = $question->get_correct_answer();
          
                  if (!$answer) {
                      return '';
                  }
                  $answer->answer = $question->vs->format_float($answer->answer, $answer->correctanswerlength, $answer->correctanswerformat);
                          
                  $response = str_replace('.', $question->ap->get_point(), $answer->answer);
                  if ($question->unitdisplay != qtype_numerical::UNITNONE) {
                      $response = $question->ap->add_unit($response);
                  }
          
                  return get_string('correctansweris', 'qtype_shortanswer', $response);
              }
          }
          

          This should solve your problem.

          The official code with all the necessary testing will come later.

          Show
          Pierre Pichet added a comment - Alan, To keep it simple for you add the function correct_response in calculated/renderer.php class qtype_calculated_renderer extends qtype_numerical_renderer { public function correct_response(question_attempt $qa) { $question = $qa->get_question(); $answer = $question->get_correct_answer(); if (!$answer) { return ''; } $answer->answer = $question->vs->format_float($answer->answer, $answer->correctanswerlength, $answer->correctanswerformat); $response = str_replace('.', $question->ap->get_point(), $answer->answer); if ($question->unitdisplay != qtype_numerical::UNITNONE) { $response = $question->ap->add_unit($response); } return get_string('correctansweris', 'qtype_shortanswer', $response); } } This should solve your problem. The official code with all the necessary testing will come later.
          Hide
          Pierre Pichet added a comment -

          The calculated->vs->format_float() is correctly tested in
          public function test_format_float_dot() {
          and
          public function test_format_float_comma()
          of class qtype_calculated_variable_substituter_test extends UnitTestCase {

          However the function was not applied in the default numerical renderer where the correct answer is the good answer.
          This is not the case in calculated where the correct answer which is the formula result, is compared within the tolerances to the student response and without any trimming that could come from the formatting ( significant figures or digits)giving the so-called good response that is displayed in the grading report.

          Supplementary testing to be added.

          Show
          Pierre Pichet added a comment - The calculated->vs->format_float() is correctly tested in public function test_format_float_dot() { and public function test_format_float_comma() of class qtype_calculated_variable_substituter_test extends UnitTestCase { However the function was not applied in the default numerical renderer where the correct answer is the good answer. This is not the case in calculated where the correct answer which is the formula result, is compared within the tolerances to the student response and without any trimming that could come from the formatting ( significant figures or digits)giving the so-called good response that is displayed in the grading report. Supplementary testing to be added.
          Hide
          Alan Arnold added a comment -

          Thanks Pierre. My colleagues at NetSpot have implemented your corrected function correct_response in calculated/renderer.php and this seems to have fixed half the problem. The correct answer is now shown to the required number of sig figs, which is great, but a submitted answer with too many sig figs is still marked as correct. See the attached screenshot for a Q that has been set for a correct answer of 3sf

          Show
          Alan Arnold added a comment - Thanks Pierre. My colleagues at NetSpot have implemented your corrected function correct_response in calculated/renderer.php and this seems to have fixed half the problem. The correct answer is now shown to the required number of sig figs, which is great, but a submitted answer with too many sig figs is still marked as correct. See the attached screenshot for a Q that has been set for a correct answer of 3sf
          Hide
          Alan Arnold added a comment - - edited

          2.1.4 screenshot showing Numerical Q type answer with too many sig figs is still shown as correct after applying fix to function correct_response in calculated/renderer.php

          Show
          Alan Arnold added a comment - - edited 2.1.4 screenshot showing Numerical Q type answer with too many sig figs is still shown as correct after applying fix to function correct_response in calculated/renderer.php
          Hide
          Pierre Pichet added a comment -

          "This is not the case in calculated where the correct answer which is the formula result, is compared within the tolerances to the student response and without any trimming that could come from the formatting ( significant figures or digits)"
          This is not a bug but a calculated question feature.
          i.e. the grading is done on the most possibly exact value from the formula not on the displayed "correct" value.

          This has been set in the original Moodle 1,4 and discussed and set in the 1,6 version after I tentatively put on the "master" the use of correct response to grade.

          Expressing the value with the correct figure or decimals is a supplementary task for which we could apply a penalty as we NOW (since 2,0) can apply a penalty for unit.

          This significant figures penalty is not implemented in the actual calculated code.

          It could be although I think that the calculated interface is already quite complex...

          It will be difficult also to define the "general consensus" about significant figures in complex equations.

          I personaly think that writing number with significant figures is a separate task and I use shortanswer question type to handle this as a separate exercise.

          Show
          Pierre Pichet added a comment - "This is not the case in calculated where the correct answer which is the formula result, is compared within the tolerances to the student response and without any trimming that could come from the formatting ( significant figures or digits)" This is not a bug but a calculated question feature. i.e. the grading is done on the most possibly exact value from the formula not on the displayed "correct" value. This has been set in the original Moodle 1,4 and discussed and set in the 1,6 version after I tentatively put on the "master" the use of correct response to grade. Expressing the value with the correct figure or decimals is a supplementary task for which we could apply a penalty as we NOW (since 2,0) can apply a penalty for unit. This significant figures penalty is not implemented in the actual calculated code. It could be although I think that the calculated interface is already quite complex... It will be difficult also to define the "general consensus" about significant figures in complex equations. I personaly think that writing number with significant figures is a separate task and I use shortanswer question type to handle this as a separate exercise.
          Hide
          Pierre Pichet added a comment -

          Tim,
          The proposed solution correct the rendering format of the good response using a specific correct_response(question_attempt $qa) function.

          Is it necessary to create a specific unittest ?

          If so, have you an example of rendering test ?

          Show
          Pierre Pichet added a comment - Tim, The proposed solution correct the rendering format of the good response using a specific correct_response(question_attempt $qa) function. Is it necessary to create a specific unittest ? If so, have you an example of rendering test ?
          Hide
          Tim Hunt added a comment -

          Most of the tests of rendering are in the files called testwalkthrough.php. so, in this case, look at question/type/calculated/simpletest/testwalkthrough.php.

          The bit you particularly want to look at is the lines that start $this->check_current_output().

          To make sense of that, you need to know about simpletest expectations. But you should be able to add a bit like
          new PatternExpectation('/' . preg_quote('...') . '/')),
          in the appropriate place.

          Show
          Tim Hunt added a comment - Most of the tests of rendering are in the files called testwalkthrough.php. so, in this case, look at question/type/calculated/simpletest/testwalkthrough.php. The bit you particularly want to look at is the lines that start $this->check_current_output(). To make sense of that, you need to know about simpletest expectations. But you should be able to add a bit like new PatternExpectation('/' . preg_quote('...') . '/')), in the appropriate place.
          Hide
          Pierre Pichet added a comment -

          Thanks for the info, a first test works OK, I will create the necessary test for testing significant figures and decimals. This imply that I use the $q->ap->getpoint() to create the number 6.. in the preg_quote.
          I will check that the test is working correctly with french as language

          Show
          Pierre Pichet added a comment - Thanks for the info, a first test works OK, I will create the necessary test for testing significant figures and decimals. This imply that I use the $q->ap->getpoint() to create the number 6.. in the preg_quote. I will check that the test is working correctly with french as language
          Hide
          Pierre Pichet added a comment -

          On testing I found that the new (2,1) function $question->vs->format_float() does not hanldle well significant figures if the response is an integer

          i.e. good response 3 significant figures 48.0 is shown as 48 although 48.1 is shown as 48.1

          This seems to be related to the way the sprintf g function handle what seem to be an integer
          // Significant figures.
          $x = sprintf('%.' . $length . 'g', $x);

          However the old question type format
          qtype_calculated_calculate_answer($formula, $individualdata,
          $tolerance, $tolerancetype, $answerlength, $answerformat='1', $unit='')

          function handle the case correctly i.e. 48.0

          My proposal is to put in question.php the old function as was done for find_formula_errors()

          Show
          Pierre Pichet added a comment - On testing I found that the new (2,1) function $question->vs->format_float() does not hanldle well significant figures if the response is an integer i.e. good response 3 significant figures 48.0 is shown as 48 although 48.1 is shown as 48.1 This seems to be related to the way the sprintf g function handle what seem to be an integer // Significant figures. $x = sprintf('%.' . $length . 'g', $x); However the old question type format qtype_calculated_calculate_answer($formula, $individualdata, $tolerance, $tolerancetype, $answerlength, $answerformat='1', $unit='') function handle the case correctly i.e. 48.0 My proposal is to put in question.php the old function as was done for find_formula_errors()
          Hide
          Adam Olley added a comment -

          Hi Tim / Pierre,

          Has any work been done on this? If so, is there some code somewhere (github/etc) to review or continue work from? If not, I'll take a look at coming up with a solution.

          Show
          Adam Olley added a comment - Hi Tim / Pierre, Has any work been done on this? If so, is there some code somewhere (github/etc) to review or continue work from? If not, I'll take a look at coming up with a solution.
          Hide
          Pierre Pichet added a comment -

          Effectively this has been put aside.
          I just retraced the work done and will look at what remains to do.
          More details tomorrow.

          Show
          Pierre Pichet added a comment - Effectively this has been put aside. I just retraced the work done and will look at what remains to do. More details tomorrow.
          Hide
          Pierre Pichet added a comment -

          I just push the last year work
          https://github.com/ppichet/moodle/tree/MDL-31680
          the compare
          https://github.com/ppichet/moodle/compare/master...MDL-31680

          The unit tests are not done and if we use this version ( $question->vs->format_float() ) a correct response like 48.0 is
          handled as an integer so is shown as 48
          Either we ignore this little limitation or we need to use the old function (see preceeding coments).

          Show
          Pierre Pichet added a comment - I just push the last year work https://github.com/ppichet/moodle/tree/MDL-31680 the compare https://github.com/ppichet/moodle/compare/master...MDL-31680 The unit tests are not done and if we use this version ( $question->vs->format_float() ) a correct response like 48.0 is handled as an integer so is shown as 48 Either we ignore this little limitation or we need to use the old function (see preceeding coments).
          Hide
          Pierre Pichet added a comment -

          You should note however that even if the correct response is shown with decimal figures set, the correct response shown
          is not used for grading.
          The grading is done on the exact response from the math formula within the tolerance limits.

          Show
          Pierre Pichet added a comment - You should note however that even if the correct response is shown with decimal figures set, the correct response shown is not used for grading. The grading is done on the exact response from the math formula within the tolerance limits.
          Hide
          Tim Hunt added a comment -

          I am not sure it is right to fix this in the renderer. Wouldn't it be better to make $question->get_correct_answer() return the correct value? (That would also make 'fill with correct' work in question preview.)

          Show
          Tim Hunt added a comment - I am not sure it is right to fix this in the renderer. Wouldn't it be better to make $question->get_correct_answer() return the correct value? (That would also make 'fill with correct' work in question preview.)
          Hide
          Pierre Pichet added a comment -

          In calculated the correct_answer is the result of the formual calculation that is used to grade using the tolerances.

          The correct response is the rounding to the digit or significant figures set by the teacher of the correct answer.
          This is why when creating datasets, we have to check that the "apparent" correct response is inside the tolerances of the correct_answer.
          This is different from the numerical handling.

          Show
          Pierre Pichet added a comment - In calculated the correct_answer is the result of the formual calculation that is used to grade using the tolerances. The correct response is the rounding to the digit or significant figures set by the teacher of the correct answer. This is why when creating datasets, we have to check that the "apparent" correct response is inside the tolerances of the correct_answer. This is different from the numerical handling.
          Hide
          Pierre Pichet added a comment -

          On further testing with the actual 2.3 the "fill with correct" use the calculation result i.e. 0.900090009...
          and the correct response shown is 0.900090009...
          In 1.9 an identical question the "fill with correct" use the correct response formatted i.e. 0.9001
          and the correct answer shown is 0.9001.
          I will update MDL-31680 to current master and do some tests tonight.

          Show
          Pierre Pichet added a comment - On further testing with the actual 2.3 the "fill with correct" use the calculation result i.e. 0.900090009... and the correct response shown is 0.900090009... In 1.9 an identical question the "fill with correct" use the correct response formatted i.e. 0.9001 and the correct answer shown is 0.9001. I will update MDL-31680 to current master and do some tests tonight.
          Hide
          Pierre Pichet added a comment -

          On looking back to numerical/question.php code where the
          get_correct_response(), get_matching_answer() and get_correct_answer() are defined,
          your suggestion to add a new get_correct_answer() in calculated/question.php is the best way to do it.

          Show
          Pierre Pichet added a comment - On looking back to numerical/question.php code where the get_correct_response(), get_matching_answer() and get_correct_answer() are defined, your suggestion to add a new get_correct_answer() in calculated/question.php is the best way to do it.
          Hide
          Pierre Pichet added a comment -

          The code has been put in calculated question.php
          unit tests to do.

          Show
          Pierre Pichet added a comment - The code has been put in calculated question.php unit tests to do.
          Hide
          Tim Hunt added a comment -

          Looking good apart from one thing: I think that

          $answer->answer = $this->vs->format_float(...

          Already uses the correct decimal point character, so you don't need

          $response = array('answer' => str_replace('.', $this->ap->get_point(), $answer->answer));

          Show
          Tim Hunt added a comment - Looking good apart from one thing: I think that $answer->answer = $this->vs->format_float(... Already uses the correct decimal point character, so you don't need $response = array('answer' => str_replace('.', $this->ap->get_point(), $answer->answer));
          Hide
          Pierre Pichet added a comment - - edited

          I apply your suggestion and I created a first phpunit test.
          As I used the simpler call to calculated/tests/question_test.php I need to add a require_once to numerical.
          I have put a first test to a 2 decimals response i.e. 3.14 instead of 3.1416
          I will add at least one more test for significant figure condition.
          Any suggestions ?

          Show
          Pierre Pichet added a comment - - edited I apply your suggestion and I created a first phpunit test. As I used the simpler call to calculated/tests/question_test.php I need to add a require_once to numerical. I have put a first test to a 2 decimals response i.e. 3.14 instead of 3.1416 I will add at least one more test for significant figure condition. Any suggestions ?
          Hide
          Tim Hunt added a comment -

          I thougt that somewhere in the code there was a unit test that forced the language to be french, and then checked the 1,00 forms, but I cannot find it now.

          You should probably add a test for a case where the computed result is an integer, which should be displayed to 2dp.

          And, as you say, sig figs.

          Show
          Tim Hunt added a comment - I thougt that somewhere in the code there was a unit test that forced the language to be french, and then checked the 1,00 forms, but I cannot find it now. You should probably add a test for a case where the computed result is an integer, which should be displayed to 2dp. And, as you say, sig figs.
          Hide
          Pierre Pichet added a comment -

          I add new tests with significant figures which indicated that the rules seem OK i.e 31.0 + 0.01 give 31.0 and not 31

          However when testing in preview a real calcualted question the result is 31 and not the expected 31.0 .

          Looking at the php info http://www.php.net/manual/en/function.sprintf.php

          A type specifier that says what type the argument data should be treated as. Possible types:

          % - a literal percent character. No argument is required.
          b - the argument is treated as an integer, and presented as a binary number.
          c - the argument is treated as an integer, and presented as the character with that ASCII value.
          d - the argument is treated as an integer, and presented as a (signed) decimal number.
          e - the argument is treated as scientific notation (e.g. 1.2e+2). The precision specifier stands for the number of digits after the decimal point since PHP 5.2.1. In earlier versions, it was taken as number of significant digits (one less).
          E - like %e but uses uppercase letter (e.g. 1.2E+2).
          u - the argument is treated as an integer, and presented as an unsigned decimal number.
          f - the argument is treated as a float, and presented as a floating-point number (locale aware).
          F - the argument is treated as a float, and presented as a floating-point number (non-locale aware). Available since PHP 4.3.10 and PHP 5.0.3.
          g - shorter of %e and %f.
          G - shorter of %E and %f.
          o - the argument is treated as an integer, and presented as an octal number.
          s - the argument is treated as and presented as a string.
          x - the argument is treated as an integer and presented as a hexadecimal number (with lowercase letters).
          X - the argument is treated as an integer and presented as a hexadecimal number (with uppercase letters).

          However in the same page

          An optional precision specifier in the form of a period (`.') followed by an optional decimal digit string that says how many decimal digits should be displayed for floating-point numbers. When using this specifier on a string, it acts as a cutoff point, setting a maximum character limit to the string.

          I temporarily conclude that the output of get_correct_response() is OK although the output in the renderer is not for the 31.0 values that are displayed as 31 .

          Show
          Pierre Pichet added a comment - I add new tests with significant figures which indicated that the rules seem OK i.e 31.0 + 0.01 give 31.0 and not 31 However when testing in preview a real calcualted question the result is 31 and not the expected 31.0 . Looking at the php info http://www.php.net/manual/en/function.sprintf.php A type specifier that says what type the argument data should be treated as. Possible types: % - a literal percent character. No argument is required. b - the argument is treated as an integer, and presented as a binary number. c - the argument is treated as an integer, and presented as the character with that ASCII value. d - the argument is treated as an integer, and presented as a (signed) decimal number. e - the argument is treated as scientific notation (e.g. 1.2e+2). The precision specifier stands for the number of digits after the decimal point since PHP 5.2.1. In earlier versions, it was taken as number of significant digits (one less). E - like %e but uses uppercase letter (e.g. 1.2E+2). u - the argument is treated as an integer, and presented as an unsigned decimal number. f - the argument is treated as a float, and presented as a floating-point number (locale aware). F - the argument is treated as a float, and presented as a floating-point number (non-locale aware). Available since PHP 4.3.10 and PHP 5.0.3. g - shorter of %e and %f. G - shorter of %E and %f. o - the argument is treated as an integer, and presented as an octal number. s - the argument is treated as and presented as a string. x - the argument is treated as an integer and presented as a hexadecimal number (with lowercase letters). X - the argument is treated as an integer and presented as a hexadecimal number (with uppercase letters). However in the same page An optional precision specifier in the form of a period (`.') followed by an optional decimal digit string that says how many decimal digits should be displayed for floating-point numbers. When using this specifier on a string, it acts as a cutoff point, setting a maximum character limit to the string. I temporarily conclude that the output of get_correct_response() is OK although the output in the renderer is not for the 31.0 values that are displayed as 31 .
          Hide
          Pierre Pichet added a comment -

          Given the 6 hours time lag between our location, you were online quite late

          The french , is tested in numerical which seems sufficient for this bug but will be treated more completely in another bug MDL-37955 and related.

          The fact that the 0 is not displayed when the significant figure rules could allow it, is a minor problem
          as the grading is not implied.
          A teacher could just explain that the computer sees 48.0 as an integer

          On code viewpoint the way the calculated/helper.php create the first calculated question object as default was found by trial and error...

          Show
          Pierre Pichet added a comment - Given the 6 hours time lag between our location, you were online quite late The french , is tested in numerical which seems sufficient for this bug but will be treated more completely in another bug MDL-37955 and related. The fact that the 0 is not displayed when the significant figure rules could allow it, is a minor problem as the grading is not implied. A teacher could just explain that the computer sees 48.0 as an integer On code viewpoint the way the calculated/helper.php create the first calculated question object as default was found by trial and error...
          Hide
          Pierre Pichet added a comment -

          I just add a new test that asserts as equal '31.0' and '31' ...

          As Captain Kirk we are at the last frontier...

          or PHP identifies that these are numbers that are effectively equals...

          Show
          Pierre Pichet added a comment - I just add a new test that asserts as equal '31.0' and '31' ... As Captain Kirk we are at the last frontier... or PHP identifies that these are numbers that are effectively equals...
          Hide
          Tim Hunt added a comment -

          I think to do that you need to use

          $this->assertSame('31.0', ... computed value here ...);

          Show
          Tim Hunt added a comment - I think to do that you need to use $this->assertSame('31.0', ... computed value here ...);
          Hide
          Pierre Pichet added a comment -

          Effectively $this->assertSame confirm that the integer (as 31) is the output from the get_correct_response.
          I update github with the last no error version.
          Should we come back to the old qtype_calculated_calculate_answer() function ?

          Show
          Pierre Pichet added a comment - Effectively $this->assertSame confirm that the integer (as 31) is the output from the get_correct_response. I update github with the last no error version. Should we come back to the old qtype_calculated_calculate_answer() function ?
          Hide
          Tim Hunt added a comment -

          Great. The code is now good. All we need are some manual testing instructions in this bug and this can be submitted for integration.

          Show
          Tim Hunt added a comment - Great. The code is now good. All we need are some manual testing instructions in this bug and this can be submitted for integration.
          Hide
          Pierre Pichet added a comment -

          Working on details, things are not so clear.
          It seems that the calculated should have its own correct_response() function in the renderer.php .

          I am also exploring how to use the qtype_calculated_calculate_answer() function
          in the process as it handles number format correctly for years.
          (I will search the tracker to confirm).

          Show
          Pierre Pichet added a comment - Working on details, things are not so clear. It seems that the calculated should have its own correct_response() function in the renderer.php . I am also exploring how to use the qtype_calculated_calculate_answer() function in the process as it handles number format correctly for years. (I will search the tracker to confirm).
          Hide
          Pierre Pichet added a comment -

          I push on github an experimental version that pass the unit test for response as 3.0 using a copy of the old code.
          I want to know if you agree with this option before cleaning and strenghthening it.

          Show
          Pierre Pichet added a comment - I push on github an experimental version that pass the unit test for response as 3.0 using a copy of the old code. I want to know if you agree with this option before cleaning and strenghthening it.
          Hide
          Tim Hunt added a comment -

          I have to say that it seems like a lot of code to do something seemingly simple.

          Also, it would be good to have unit tests to prove it is doing the right thing.

          Show
          Tim Hunt added a comment - I have to say that it seems like a lot of code to do something seemingly simple. Also, it would be good to have unit tests to prove it is doing the right thing.
          Hide
          Pierre Pichet added a comment -

          The only real bad case is when the correct answer is a value with 0 as only decimal as 31.0 and the setting is significant figures (here 3)
          Somehow the sprint g option no more works corectly as setting the significant figures.
          As they wrote in php srintf docs.
          e - the argument is treated as scientific notation (e.g. 1.2e+2). The precision specifier stands for the number of digits after the decimal point since PHP 5.2.1. In earlier versions, it was taken as number of significant digits (one less).

          This is more occasional or a nuisance than a major bug.

          However this code is a modifed copy of the qtype_calculated_calculate_answer function that is only called twice for
          the display of the correct answer in datasetitems_form.

          I could probably divise the code of qtype_calculated_calculate_answer() to create a function that can be used by the get_correct_response, this will remove the need to duplicate this complex code.

          More news about this probably monday...

          Show
          Pierre Pichet added a comment - The only real bad case is when the correct answer is a value with 0 as only decimal as 31.0 and the setting is significant figures (here 3) Somehow the sprint g option no more works corectly as setting the significant figures. As they wrote in php srintf docs. e - the argument is treated as scientific notation (e.g. 1.2e+2). The precision specifier stands for the number of digits after the decimal point since PHP 5.2.1. In earlier versions, it was taken as number of significant digits (one less). This is more occasional or a nuisance than a major bug. However this code is a modifed copy of the qtype_calculated_calculate_answer function that is only called twice for the display of the correct answer in datasetitems_form. I could probably divise the code of qtype_calculated_calculate_answer() to create a function that can be used by the get_correct_response, this will remove the need to duplicate this complex code. More news about this probably monday...
          Hide
          Pierre Pichet added a comment - - edited

          Tim,
          I need the function format_float() actually in calculated/question.php class qtype_calculated_variable_substituter {

          also in datasetedit_form.php.
          It seems that it should then be put in calculated/questiontype.php
          And it is also call by the renderer.
          One solution is to initialized a new parameter $questiontype in qtype_calculated_variable_substituter construct.

          What do you suggest ?

          Show
          Pierre Pichet added a comment - - edited Tim, I need the function format_float() actually in calculated/question.php class qtype_calculated_variable_substituter { also in datasetedit_form.php. It seems that it should then be put in calculated/questiontype.php And it is also call by the renderer. One solution is to initialized a new parameter $questiontype in qtype_calculated_variable_substituter construct. What do you suggest ?
          Hide
          Pierre Pichet added a comment -

          On a closer look I just realize how new calculated question object is separate from the old calculated questiontype question.

          As an example you need to duplicate in question.php the calculated questiontype
          function qtype_calculated_find_formula_errors($formula) {
          // Validates the formula submitted from the question edit page.
          // Returns false if everything is alright.
          // Otherwise it constructs an error message
          // Strip away dataset namespublic function get_formula_errors($formula) {
          as
          public function get_formula_errors($formula) {
          // Validates the formula submitted from the question edit page.
          // Returns false if everything is alright.
          // Otherwise it constructs an error message.
          // Strip away dataset names.

          So we need a similar doubling of the format_float() function.

          For this last function I realize that for the format 1 => decimalformat, your solution to use sprintf is a good one
          if ($format == 1) {

          • // Decimal places.
          • $x = sprintf('%.' . $length . 'F', $x);
            So I will eliminate the old code for this $format case in the old code and keep yours in the new engine code.

          " a lot of code " will be less true

          Show
          Pierre Pichet added a comment - On a closer look I just realize how new calculated question object is separate from the old calculated questiontype question. As an example you need to duplicate in question.php the calculated questiontype function qtype_calculated_find_formula_errors($formula) { // Validates the formula submitted from the question edit page. // Returns false if everything is alright. // Otherwise it constructs an error message // Strip away dataset namespublic function get_formula_errors($formula) { as public function get_formula_errors($formula) { // Validates the formula submitted from the question edit page. // Returns false if everything is alright. // Otherwise it constructs an error message. // Strip away dataset names. So we need a similar doubling of the format_float() function. For this last function I realize that for the format 1 => decimalformat, your solution to use sprintf is a good one if ($format == 1) { // Decimal places. $x = sprintf('%.' . $length . 'F', $x); So I will eliminate the old code for this $format case in the old code and keep yours in the new engine code. " a lot of code " will be less true
          Hide
          Pierre Pichet added a comment -

          The last version seems OK.
          Tests passed OK.
          TO DO questions to test.

          Show
          Pierre Pichet added a comment - The last version seems OK. Tests passed OK. TO DO questions to test.
          Hide
          Pierre Pichet added a comment - - edited

          Its midnight, things got clearer so hopefully last tests tomorrow

          Show
          Pierre Pichet added a comment - - edited Its midnight, things got clearer so hopefully last tests tomorrow
          Hide
          Pierre Pichet added a comment -

          So solving calculatedsimple dataset problem discovered when testing MDL-31680 on copy of calculatedsimple.
          This will, by the side, allow more easy testing of MDL-31680.
          Is it necessary to create a double issue on github for the subtask or can I include the solution in MDL-31680 branch ?

          Show
          Pierre Pichet added a comment - So solving calculatedsimple dataset problem discovered when testing MDL-31680 on copy of calculatedsimple. This will, by the side, allow more easy testing of MDL-31680 . Is it necessary to create a double issue on github for the subtask or can I include the solution in MDL-31680 branch ?
          Hide
          Pierre Pichet added a comment -

          In fact this calculatedsimple saving appears to be solved by just one line of code.

              public function save_question($question, $form) {
                   global $DB;
          -        if ($this->wizardpagesnumber() == 1) {
          +        if ($this->wizardpagesnumber() == 1  || $question->qtype == 'calculatedsimple') {
                           $question = parent::save_question($question, $form);
                       return $question;
                   }
           
                   $wizardnow =  optional_param('wizardnow', '', PARAM_ALPHA);
          
          Show
          Pierre Pichet added a comment - In fact this calculatedsimple saving appears to be solved by just one line of code. public function save_question($question, $form) { global $DB; - if ($this->wizardpagesnumber() == 1) { + if ($this->wizardpagesnumber() == 1 || $question->qtype == 'calculatedsimple') { $question = parent::save_question($question, $form); return $question; } $wizardnow = optional_param('wizardnow', '', PARAM_ALPHA);
          Hide
          Pierre Pichet added a comment -

          There is lot to test on master before creating the other versions branchs.

          Show
          Pierre Pichet added a comment - There is lot to test on master before creating the other versions branchs.
          Hide
          Pierre Pichet added a comment - - edited

          The https://tracker.moodle.org/secure/attachment/31662/questions-CHI0310_100-test%20MDL-31680-20130313-1325.xml

          contains 20 questions : calculatedsimple questions and the correspondiong regular calculated questions in moodleXML (moodle23) format.
          There is no category defined in the import file.
          obtained by save as regular feature of calculatedsimple.
          Use in preview fill the good answer and look at the correct response on submit.
          Look also at the values diplayed on set(s) of wild card(s) values (in calculated they appears on the third editing page).
          They should be similar to the other although they come from a different function.

          Show
          Pierre Pichet added a comment - - edited The https://tracker.moodle.org/secure/attachment/31662/questions-CHI0310_100-test%20MDL-31680-20130313-1325.xml contains 20 questions : calculatedsimple questions and the correspondiong regular calculated questions in moodleXML (moodle23) format. There is no category defined in the import file. obtained by save as regular feature of calculatedsimple. Use in preview fill the good answer and look at the correct response on submit. Look also at the values diplayed on set(s) of wild card(s) values (in calculated they appears on the third editing page). They should be similar to the other although they come from a different function.
          Hide
          Tim Hunt added a comment -

          Perhaps I am being excessively fussy, but I still think that the code could be better. For example.

          1. Rather than while loops, can't we use log10 to calculate the correct exponent immediately https://github.com/ppichet/moodle/compare/master...MDL-31680#L0R351

          2. Then we have some almost duplicated code, and anyway, can't we format the main part of the answer with a recursive call to format_float using $format = 1?

          3. $exponent = 'e'.--$p10; would be better as $exponent = 'e' . ($p10 - 1);

          Show
          Tim Hunt added a comment - Perhaps I am being excessively fussy, but I still think that the code could be better. For example. 1. Rather than while loops, can't we use log10 to calculate the correct exponent immediately https://github.com/ppichet/moodle/compare/master...MDL-31680#L0R351 2. Then we have some almost duplicated code, and anyway, can't we format the main part of the answer with a recursive call to format_float using $format = 1? 3. $exponent = 'e'.--$p10; would be better as $exponent = 'e' . ($p10 - 1);
          Hide
          Pierre Pichet added a comment -

          Your not fussy but you are a code expert and in 2013 you would not write the code that way.

          I am not a code expert but I use the rule "if its works don't fix it"...

          Somebody in PHP team change the rule for sprintf with G option so that it does not anymore handle correctly significant figure.
          This is why format_float() needs to be modified.
          And I choose to duplicate the old code in the new format_float().

          1. I suspect that the while loops are coming from the C code of log10. As this is compiled this could be faster.
          However we need to handle numbers at the precision limit of the actual PHP intallation.
          Some moodle users are quite fussy about the precision limit.
          Having the detailed code could be an advantage.

          2. I am not sure how to do "a recursive call to format_float" as the qtype_calculated_calculate_answer($formula, $individualdata,$tolerance, $tolerancetype, $answerlength, $answerformat = '1', $unit = '') { is also used in the
          edit_calculatedsimple_form.php and the datasetitems_form.php.

          3. I suspect that using --$p10 is a garantee that the number will be an integer which is what we need.

          Show
          Pierre Pichet added a comment - Your not fussy but you are a code expert and in 2013 you would not write the code that way. I am not a code expert but I use the rule "if its works don't fix it"... Somebody in PHP team change the rule for sprintf with G option so that it does not anymore handle correctly significant figure. This is why format_float() needs to be modified. And I choose to duplicate the old code in the new format_float(). 1. I suspect that the while loops are coming from the C code of log10. As this is compiled this could be faster. However we need to handle numbers at the precision limit of the actual PHP intallation. Some moodle users are quite fussy about the precision limit. Having the detailed code could be an advantage. 2. I am not sure how to do "a recursive call to format_float" as the qtype_calculated_calculate_answer($formula, $individualdata,$tolerance, $tolerancetype, $answerlength, $answerformat = '1', $unit = '') { is also used in the edit_calculatedsimple_form.php and the datasetitems_form.php. 3. I suspect that using --$p10 is a garantee that the number will be an integer which is what we need.
          Hide
          Pierre Pichet added a comment -

          Following these comments what are your recommandations ?

          Show
          Pierre Pichet added a comment - Following these comments what are your recommandations ?
          Hide
          Tim Hunt added a comment -

          Then I guess the current version of the patch is OK. Do you think there is anything else to do before submitting for integration?

          Show
          Tim Hunt added a comment - Then I guess the current version of the patch is OK. Do you think there is anything else to do before submitting for integration?
          Hide
          Pierre Pichet added a comment -

          I will create the 23 and 24 version tonight unless you want to apply this only to master
          and complete the instructions for testing using the import file.
          Tomorrow everything should be ready.

          Show
          Pierre Pichet added a comment - I will create the 23 and 24 version tonight unless you want to apply this only to master and complete the instructions for testing using the import file. Tomorrow everything should be ready.
          Hide
          Pierre Pichet added a comment -

          I tested the questions in the files in MOODLE_STABLE, in MOODLE_23_STABLE and in MOODLE_24_STABLE
          The unit tests were done only on master.
          The values were ok in the preview and when editing.

          Show
          Pierre Pichet added a comment - I tested the questions in the files in MOODLE_STABLE, in MOODLE_23_STABLE and in MOODLE_24_STABLE The unit tests were done only on master. The values were ok in the preview and when editing.
          Hide
          Tim Hunt added a comment -

          Great. Thanks Pierre. Submitting for integration now.

          Show
          Tim Hunt added a comment - Great. Thanks Pierre. Submitting for integration now.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Sorry I'm reopening this, whitespace and friends. See, for master:

          http://stronk7.doesntexist.com/job/Precheck%20remote%20branch/lastSuccessfulBuild/artifact/work/smurf.xml

          Also, plz, in the future don't mix the solution to a bug with dozens of style changes, it makes really difficult to analyze the solution.

          Finally, Tim, are you ok to backport this to 23 and 24 ?

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Sorry I'm reopening this, whitespace and friends. See, for master: http://stronk7.doesntexist.com/job/Precheck%20remote%20branch/lastSuccessfulBuild/artifact/work/smurf.xml Also, plz, in the future don't mix the solution to a bug with dozens of style changes, it makes really difficult to analyze the solution. Finally, Tim, are you ok to backport this to 23 and 24 ? Ciao
          Hide
          CiBoT added a comment -

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

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

          Sorry for the mix of stylechanges and real code.

          This happens as the files was either an old one (like calculated/questiontype.php) or
          written before the code checker (like calculated/question.php)

          Should I create a separate issue as a subtask for the style changes before re-submit ?

          As a specific comment the code checker does not handle well SQL on multiple lines like

                 $categorydatasetdefs = $DB->get_records_sql(
                      "SELECT  a.*
                         FROM {question_datasets} b, {question_dataset_definitions} a
                        WHERE a.id = b.datasetdefinition
                          AND a.type = '1'
                          AND a.category != 0
                          AND b.question = ?
                     ORDER BY a.name ", array($question->id));
                  $questionname = $question->name; 
          
          giving
          
          ············"SELECT··a.*
          543: Whitespace found at end of line within string
          ···············FROM·{question_datasets}·b,·{question_dataset_definitions}·a
          544: Whitespace found at end of line within string
          ··············WHERE·a.id·=·b.datasetdefinition
          545: Whitespace found at end of line within string
          ················AND·a.type·=·'1'
          546: Whitespace found at end of line within string
          ················AND·a.category·!=·0
          547: Whitespace found at end of line within string
          ················AND·b.question·=·?
          548: Whitespace found at end of line within string
          
          
          Show
          Pierre Pichet added a comment - Sorry for the mix of stylechanges and real code. This happens as the files was either an old one (like calculated/questiontype.php) or written before the code checker (like calculated/question.php) Should I create a separate issue as a subtask for the style changes before re-submit ? As a specific comment the code checker does not handle well SQL on multiple lines like $categorydatasetdefs = $DB->get_records_sql( "SELECT a.* FROM {question_datasets} b, {question_dataset_definitions} a WHERE a.id = b.datasetdefinition AND a.type = '1' AND a.category != 0 AND b.question = ? ORDER BY a.name ", array($question->id)); $questionname = $question->name; giving ············"SELECT··a.* 543: Whitespace found at end of line within string ···············FROM·{question_datasets}·b,·{question_dataset_definitions}·a 544: Whitespace found at end of line within string ··············WHERE·a.id·=·b.datasetdefinition 545: Whitespace found at end of line within string ················AND·a.type·=·'1' 546: Whitespace found at end of line within string ················AND·a.category·!=·0 547: Whitespace found at end of line within string ················AND·b.question·=·? 548: Whitespace found at end of line within string
          Hide
          Pierre Pichet added a comment - - edited

          As for 23 or 24 this code do not change the internal answers values and the grading just the way they appear to the user when he answer the question.

          In fact the code put back the display as it was before the new engine code.

          In the actual code( master, 23 , 24) when creating a calculated or calculatedsimple question as

          {x}

          +0.012345 , in the editing form the following is shown for 3 significant figures

          45+0.012345 = 45.0 which is the correct answer

          The 45.0 is obtained by the old code.

          When you preview the code following the new engine code
          the following appears

          The correct answer is: 45.012345

          The rounding to 3 significant figures is not done mostly because PHP sprintf() function does no more allow rounding to significant figures.

          If the student answer with the exact correct response he will receive full grade.

          The proposal is to correct the display of the correct answer when the question is used

          i.e 45.0 .

          The grading will not changed as the question as been verified when editing that the
          correctly rounded value (i.e. 45.0) is within the tolerance set for the exact answer.

          Show
          Pierre Pichet added a comment - - edited As for 23 or 24 this code do not change the internal answers values and the grading just the way they appear to the user when he answer the question. In fact the code put back the display as it was before the new engine code. In the actual code( master, 23 , 24) when creating a calculated or calculatedsimple question as {x} +0.012345 , in the editing form the following is shown for 3 significant figures 45+0.012345 = 45.0 which is the correct answer The 45.0 is obtained by the old code. When you preview the code following the new engine code the following appears The correct answer is: 45.012345 The rounding to 3 significant figures is not done mostly because PHP sprintf() function does no more allow rounding to significant figures. If the student answer with the exact correct response he will receive full grade. The proposal is to correct the display of the correct answer when the question is used i.e 45.0 . The grading will not changed as the question as been verified when editing that the correctly rounded value (i.e. 45.0) is within the tolerance set for the exact answer.
          Hide
          Pierre Pichet added a comment - - edited

          master has been updated.
          I think I eliminate all the code checker errors but not probably in the best
          style.

          Looking around PHPDocs I conclude that the phpDoc detect that the new functions are not documented.
          Should all new functions be documented from now ?

          Show
          Pierre Pichet added a comment - - edited master has been updated. I think I eliminate all the code checker errors but not probably in the best style. Looking around PHPDocs I conclude that the phpDoc detect that the new functions are not documented. Should all new functions be documented from now ?
          Hide
          Tim Hunt added a comment -

          You should document any new function you write, but not if you are overriding a method from a base class (since the base class will document what that method should do.)

          Show
          Tim Hunt added a comment - You should document any new function you write, but not if you are overriding a method from a base class (since the base class will document what that method should do.)
          Hide
          Pierre Pichet added a comment -

          So we will wait for this week version and rebase every version before submitting again ?

          Show
          Pierre Pichet added a comment - So we will wait for this week version and rebase every version before submitting again ?
          Hide
          Tim Hunt added a comment -

          Yes. The weekly should be out soon.

          Show
          Tim Hunt added a comment - Yes. The weekly should be out soon.
          Hide
          Pierre Pichet added a comment -

          I just rebase the master version.
          Given the remarks on the numerous changes, could you verify them on your installation if everything
          (code checker and PHPtests) is Ok before I rebase 23 and 24 versions.
          Thanks

          Show
          Pierre Pichet added a comment - I just rebase the master version. Given the remarks on the numerous changes, could you verify them on your installation if everything (code checker and PHPtests) is Ok before I rebase 23 and 24 versions. Thanks
          Hide
          Pierre Pichet added a comment -

          rebase has been done on 23 and 24.

          Show
          Pierre Pichet added a comment - rebase has been done on 23 and 24.
          Hide
          Pierre Pichet added a comment -

          I create a sub task and will resubmit after

          Show
          Pierre Pichet added a comment - I create a sub task and will resubmit after
          Hide
          Adam Olley added a comment - - edited

          Testing the 2.3 based version of Pierre's patch, if the question is setup as needing 3 significant figures, and you provide an answer with 5 significant figures, it's marked as correct. This doesn't appear to be the desired behaviour to me?

          Show
          Adam Olley added a comment - - edited Testing the 2.3 based version of Pierre's patch, if the question is setup as needing 3 significant figures, and you provide an answer with 5 significant figures, it's marked as correct. This doesn't appear to be the desired behaviour to me?
          Hide
          Pierre Pichet added a comment - - edited

          The calculated questions (calculated and calculatedsimple) grade using the tolerance parameters.
          The "correct response" which is not used in the grading and which is shown to the student, should however be inside the tolerance values.
          The significant figures or decimals allow the teacher to show a response that is within the tolerances and written following the concept of significant figures although there is no grading on the significant figures.
          This is why when the teacher create the datasets (i.e. datasetitems_form.php) there is always a verification that the "correct response" is inside the tolerances.
          So "if the question is setup as needing 3 significant figures, and you provide an answer with 5 significant figures" is OK.

          Show
          Pierre Pichet added a comment - - edited The calculated questions (calculated and calculatedsimple) grade using the tolerance parameters. The "correct response" which is not used in the grading and which is shown to the student, should however be inside the tolerance values. The significant figures or decimals allow the teacher to show a response that is within the tolerances and written following the concept of significant figures although there is no grading on the significant figures. This is why when the teacher create the datasets (i.e. datasetitems_form.php) there is always a verification that the "correct response" is inside the tolerances. So "if the question is setup as needing 3 significant figures, and you provide an answer with 5 significant figures" is OK.
          Hide
          Pierre Pichet added a comment -

          The actual code of MDL-31680 contains a lot of changes to the // inline comments.
          The sub-task MDL-38792 last proposal for correcting all question/type files have a more recent version of those //in-line comments.
          What is the best (simplest) way to handle this ?

          Show
          Pierre Pichet added a comment - The actual code of MDL-31680 contains a lot of changes to the // inline comments. The sub-task MDL-38792 last proposal for correcting all question/type files have a more recent version of those //in-line comments. What is the best (simplest) way to handle this ?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          first landing wins. 2nd has to adapt. you can specify any dependency between issues by making link (and noting it in the "submit to integration comment"): A is blocking B

          (that means that A will be the 1st always to land and B the 2nd, so you can safely build B on top of A, aka branch B from A).

          Show
          Eloy Lafuente (stronk7) added a comment - first landing wins. 2nd has to adapt. you can specify any dependency between issues by making link (and noting it in the "submit to integration comment"): A is blocking B (that means that A will be the 1st always to land and B the 2nd, so you can safely build B on top of A, aka branch B from A).
          Hide
          Pierre Pichet added a comment -

          A should be the sub-task MDL-38792 as there is almost no real code changes mostly in-line comments.

          I will then be able to clean MDL-31680 of those in-line comments change that
          " it makes really difficult to analyze the solution."

          Show
          Pierre Pichet added a comment - A should be the sub-task MDL-38792 as there is almost no real code changes mostly in-line comments. I will then be able to clean MDL-31680 of those in-line comments change that " it makes really difficult to analyze the solution."
          Hide
          Tim Hunt added a comment -

          I think this is waiting for MDL-38792, so taking it out of peer review for now.

          Show
          Tim Hunt added a comment - I think this is waiting for MDL-38792 , so taking it out of peer review for now.
          Hide
          Pierre Pichet added a comment -

          I clean MDL-31680 of all in-line comments correction that will be solved by MDL-38793 so that they can be applied independently.

          next steps: do the rebase to master and description of the modifications done in MDL-31680.

          The discussion about setType MDL-38741 let me realized that the datasetitem value is not a PARAM_FLOAT but a PARAM_TEXT that contains a formatted number that will pass the test is_number().

          I modify the code accordingly and the forms (datasetitems, edit_calculatedsimple) that handle these.

          Show
          Pierre Pichet added a comment - I clean MDL-31680 of all in-line comments correction that will be solved by MDL-38793 so that they can be applied independently. next steps: do the rebase to master and description of the modifications done in MDL-31680 . The discussion about setType MDL-38741 let me realized that the datasetitem value is not a PARAM_FLOAT but a PARAM_TEXT that contains a formatted number that will pass the test is_number(). I modify the code accordingly and the forms (datasetitems, edit_calculatedsimple) that handle these.
          Hide
          Pierre Pichet added a comment -

          The rebase is done on MDL-31680

          The code proposal
          1. Solve correctly the correct response which necessitatse a quite complete review of the calculations implied.

          2.The correct response is also used when creating the dataset items ( datasetitems_form and edit_calculatedsimple_form) as
          we need to check that the correct response is inside the tolerance of the true result from the calculated formula.
          When testing this process I detect that the 2 forms do not handle correctly the errors if the user wrote a bad number when
          editing a dataitem value. The display was not updated correctly.
          The main problem was that some buttons where set as registerNoSubmitButton().
          What I learn in the process is that the main effect of a NoSubmitButton() is that it bypass the form validation.
          The actual proposal give a correct handling.

          However the problem of moodleform TYPE validation on which you are working with others is not solved at the time being.

          Either we stop and wait or we submit this on master on an experimental base as a testcase ?

          Show
          Pierre Pichet added a comment - The rebase is done on MDL-31680 The code proposal 1. Solve correctly the correct response which necessitatse a quite complete review of the calculations implied. 2.The correct response is also used when creating the dataset items ( datasetitems_form and edit_calculatedsimple_form) as we need to check that the correct response is inside the tolerance of the true result from the calculated formula. When testing this process I detect that the 2 forms do not handle correctly the errors if the user wrote a bad number when editing a dataitem value. The display was not updated correctly. The main problem was that some buttons where set as registerNoSubmitButton(). What I learn in the process is that the main effect of a NoSubmitButton() is that it bypass the form validation. The actual proposal give a correct handling. However the problem of moodleform TYPE validation on which you are working with others is not solved at the time being. Either we stop and wait or we submit this on master on an experimental base as a testcase ?
          Hide
          Tim Hunt added a comment -

          I think it is simplest to wait until they fix the setType problem (which I hope will be very soon) and then finish this.

          Show
          Tim Hunt added a comment - I think it is simplest to wait until they fix the setType problem (which I hope will be very soon) and then finish this.
          Hide
          Pierre Pichet added a comment -

          Tim,
          Although setType should be solved for 2,5, have you any news about this?
          The setType is also involved in other bugs on calculated that I want to solve this month as UQAM is migrating on 2,3
          for which support will end by june.

          Show
          Pierre Pichet added a comment - Tim, Although setType should be solved for 2,5, have you any news about this? The setType is also involved in other bugs on calculated that I want to solve this month as UQAM is migrating on 2,3 for which support will end by june.
          Hide
          Tim Hunt added a comment -

          All the setType things I know about have been fixed. Please carry on and produce a finished patch that I can peer review.

          Show
          Tim Hunt added a comment - All the setType things I know about have been fixed. Please carry on and produce a finished patch that I can peer review.
          Hide
          Pierre Pichet added a comment -

          I thought that this setType problem was related to MDL-38885 and was waiting for its closing which is not the case apparently.

          I will wait for this week release to rebase and do a final check.

          Show
          Pierre Pichet added a comment - I thought that this setType problem was related to MDL-38885 and was waiting for its closing which is not the case apparently. I will wait for this week release to rebase and do a final check.
          Hide
          Tim Hunt added a comment -

          You should not need to wait for MDL-38885.

          Show
          Tim Hunt added a comment - You should not need to wait for MDL-38885.
          Hide
          Pierre Pichet added a comment -

          Doing the rebase for the 20130513 version, I realized that the answer field as been increased setSize(55);

          This will normally result in a general increase of the formulas length and the datasetitems_form.php need some
          recoding to adjust the display although this not change anything in the answers values or other questions parameters.

          I did minimal display modifications to this issue (MDL-31680) and the fine tuning will be completed in MDL-31056.

          I also removed the calculated/questiontype.php multichoice_comment_on_datasetitems() function as no more used
          ( confirmed by http://xref.schoolsict.net/moodle/2.3/nav.html?)

          I updated the master on github.

          TO DO : complete testing (i.e. redo unit tests ) with additional testing instructions on the rendering when editing the questions with long answer formulas.

          Show
          Pierre Pichet added a comment - Doing the rebase for the 20130513 version, I realized that the answer field as been increased setSize(55); This will normally result in a general increase of the formulas length and the datasetitems_form.php need some recoding to adjust the display although this not change anything in the answers values or other questions parameters. I did minimal display modifications to this issue ( MDL-31680 ) and the fine tuning will be completed in MDL-31056 . I also removed the calculated/questiontype.php multichoice_comment_on_datasetitems() function as no more used ( confirmed by http://xref.schoolsict.net/moodle/2.3/nav.html? ) I updated the master on github. TO DO : complete testing (i.e. redo unit tests ) with additional testing instructions on the rendering when editing the questions with long answer formulas.
          Hide
          Pierre Pichet added a comment -

          Three questions illustrating the rendering of long formulas when editing.

          Show
          Pierre Pichet added a comment - Three questions illustrating the rendering of long formulas when editing.
          Hide
          Pierre Pichet added a comment -

          The master is updated with the last version.

          If everything is OK, I will merge to other versions.

          Show
          Pierre Pichet added a comment - The master is updated with the last version. If everything is OK, I will merge to other versions.
          Hide
          Tim Hunt added a comment -

          Why do the versions for the stable branch change more code, but fewer files?

          Master: 9 changed files with 213 additions and 125 deletions.
          STABLE: 5 changed files with 349 additions and 237 deletions.

          Show
          Tim Hunt added a comment - Why do the versions for the stable branch change more code, but fewer files? Master: 9 changed files with 213 additions and 125 deletions. STABLE: 5 changed files with 349 additions and 237 deletions.
          Hide
          Tim Hunt added a comment -

          2. I don't like the bit:

          if (is_string($answer)) {
              $ans = $answer;
          } else {
              $ans = $answer->answer;
          } 
          

          why is it that we don't know the form of $answer? Surely it should always be the same type of thing (string or object).

          3. Just changing PARAM_FLOAT or PARAM_INT to PARAM_TEXT must be wrong. I assume this is to do with handling , for decimal point, but to make that work, we just apply unformat float somewhere, and I can't see where that happens.

          4. It would be easier to review if you fixed one bug at a time, and submitted each fix separately.

          Thanks for working on this. It looks like it is nearly right.

          Show
          Tim Hunt added a comment - 2. I don't like the bit: if (is_string($answer)) { $ans = $answer; } else { $ans = $answer->answer; } why is it that we don't know the form of $answer? Surely it should always be the same type of thing (string or object). 3. Just changing PARAM_FLOAT or PARAM_INT to PARAM_TEXT must be wrong. I assume this is to do with handling , for decimal point, but to make that work, we just apply unformat float somewhere, and I can't see where that happens. 4. It would be easier to review if you fixed one bug at a time, and submitted each fix separately. Thanks for working on this. It looks like it is nearly right.
          Hide
          Tim Hunt added a comment -

          As usual, I forgot to click the buttons. Updating workflow state.

          Show
          Tim Hunt added a comment - As usual, I forgot to click the buttons. Updating workflow state.
          Hide
          Pierre Pichet added a comment -

          Thanks for your comments given your high workload.

          I will give more detailed comments tonight and rebase the STABLE versions.

          P.S. a preceeding comment that was effectively lost among the others:

          "The master is updated with the last version.
          If everything is OK, I will merge to other versions."

          Show
          Pierre Pichet added a comment - Thanks for your comments given your high workload. I will give more detailed comments tonight and rebase the STABLE versions. P.S. a preceeding comment that was effectively lost among the others: "The master is updated with the last version. If everything is OK, I will merge to other versions."
          Hide
          Tim Hunt added a comment -

          Ah, I see. I should ignore the other branches for now. Thanks.

          Show
          Tim Hunt added a comment - Ah, I see. I should ignore the other branches for now. Thanks.
          Hide
          Pierre Pichet added a comment -

          2."I don't like the bit:

          if (is_string($answer)) {
              $ans = $answer;
          } else {
              $ans = $answer->answer;
          } 
          

          "
          Effectively only the second option is OK.
          The structure is effectively the question answer structure as coming from get_question_options()
          and not from a preceeding form element.
          I will correct.

          Show
          Pierre Pichet added a comment - 2."I don't like the bit: if (is_string($answer)) { $ans = $answer; } else { $ans = $answer->answer; } " Effectively only the second option is OK. The structure is effectively the question answer structure as coming from get_question_options() and not from a preceeding form element. I will correct.
          Hide
          Pierre Pichet added a comment -

          3. Just changing PARAM_FLOAT or PARAM_INT to PARAM_TEXT must be wrong. I assume this is to do with handling , for decimal point, but to make that work, we just apply unformat float somewhere, and I can't see where that happens.

          The param value i.e.

          {x}

          =1.234 is not a stored as a numerical or a float but as TEXT that will be put in the equation and read by PHP
          Using PARAM_INT strip all decimals.
          Using unformat_float will solve only one of the problem about bad number and we don't want to allow the user to
          use , for numbers in the formula.

          So I choose to validate the value.

          Incidently setting I discover that the old validation messages need to be corrected.

          We need to implement a validation that will verify that the user does not put any wrong term (hack).
          The validation was not done when the user click on Update datasets parameter or simply use ENTER key.
          So I remove $mform->registerNoSubmitButton('updatedatasets');
          as the validation are done only when a real submit is done.
          The validation is done correctly when the user change a value of one wild card and instinctively hit the return key.
          This activate the first button on the page ('updatedatasets') which is now active and the values are updated or not if there is an error.

          I just retest some of the side effects of the removal of $mform->registerNoSubmsitButton('updatedatasets');
          and I am not satified.
          So more later ...

          Show
          Pierre Pichet added a comment - 3. Just changing PARAM_FLOAT or PARAM_INT to PARAM_TEXT must be wrong. I assume this is to do with handling , for decimal point, but to make that work, we just apply unformat float somewhere, and I can't see where that happens. The param value i.e. {x} =1.234 is not a stored as a numerical or a float but as TEXT that will be put in the equation and read by PHP Using PARAM_INT strip all decimals. Using unformat_float will solve only one of the problem about bad number and we don't want to allow the user to use , for numbers in the formula. So I choose to validate the value. Incidently setting I discover that the old validation messages need to be corrected. We need to implement a validation that will verify that the user does not put any wrong term (hack). The validation was not done when the user click on Update datasets parameter or simply use ENTER key. So I remove $mform->registerNoSubmitButton('updatedatasets'); as the validation are done only when a real submit is done. The validation is done correctly when the user change a value of one wild card and instinctively hit the return key. This activate the first button on the page ('updatedatasets') which is now active and the values are updated or not if there is an error. I just retest some of the side effects of the removal of $mform->registerNoSubmsitButton('updatedatasets'); and I am not satified. So more later ...
          Hide
          Tim Hunt added a comment -

          It is OK to change PARAM_FLOAT if you are going to validate it yourself, but:

          • In this case, change it to PARAM_RAW.
          • Add a comment where you call setType saying that this will be validated properly in the validation() method.
          Show
          Tim Hunt added a comment - It is OK to change PARAM_FLOAT if you are going to validate it yourself, but: In this case, change it to PARAM_RAW. Add a comment where you call setType saying that this will be validated properly in the validation() method.
          Hide
          Pierre Pichet added a comment -

          This could be a final? proposition

          • I modify PARAM_FLOAT to PARAM_RAW following your suggestion.
          • I did not modify the code flow of datasetitems_form.php . This will be done elsewhere.
          • I fix the answer test . The testing of answer as string was added when developping calculated simple but no more useful now.
          • I discover that param number was not validated in edit_calculatedsimple_form.php so I add it.

          If you agree with the last modifications,

          • I will rebase to 2.5 2.4 2.3.
          • Finalize the testing instructions.
          Show
          Pierre Pichet added a comment - This could be a final? proposition I modify PARAM_FLOAT to PARAM_RAW following your suggestion. I did not modify the code flow of datasetitems_form.php . This will be done elsewhere. I fix the answer test . The testing of answer as string was added when developping calculated simple but no more useful now. I discover that param number was not validated in edit_calculatedsimple_form.php so I add it. If you agree with the last modifications, I will rebase to 2.5 2.4 2.3. Finalize the testing instructions.
          Hide
          Tim Hunt added a comment -

          That sounds good. Thanks.

          Show
          Tim Hunt added a comment - That sounds good. Thanks.
          Hide
          Tim Lock added a comment -

          Hi Pierre,

          When do you think you will be able to update the 2.3.x github code with your latest changes so I can test it?

          Show
          Tim Lock added a comment - Hi Pierre, When do you think you will be able to update the 2.3.x github code with your latest changes so I can test it?
          Hide
          Pierre Pichet added a comment -

          Yes,
          I miss the Tim agreement, so I will rebase most probably today.

          Show
          Pierre Pichet added a comment - Yes, I miss the Tim agreement, so I will rebase most probably today.
          Hide
          Pierre Pichet added a comment -

          MDL-31680_23 rebase to last week.

          Show
          Pierre Pichet added a comment - MDL-31680 _23 rebase to last week.
          Hide
          Pierre Pichet added a comment - - edited

          Tim,

          Closing for holidays June 15 - Aug 15 as I will not have access to nothing more than an Ipad in this period.

          This weekend I will rebase to this week moodle versions including 2.3 as I just note jour last comment.

          If something goes wrong, I could possibly correct it in the next week.

          Show
          Pierre Pichet added a comment - - edited Tim, Closing for holidays June 15 - Aug 15 as I will not have access to nothing more than an Ipad in this period. This weekend I will rebase to this week moodle versions including 2.3 as I just note jour last comment. If something goes wrong, I could possibly correct it in the next week.
          Hide
          Tim Hunt added a comment -

          1. I am worried by the

          $ans = $answer->answer;
          if (strlen($ans) > 20) {
              $ans = substr($ans, 0, 17).'...';
          }
          

          part. Why is thix necssary? Surely answers will never be very long and truncating floating point numbers like this seems like a bad idea. (If truncating them was a good idea, you should use the shorten_text function, rather then your own code.)

          This comment applies in quite a lot of places in the code, where essentially the same thing has been duplicated.

          2. There is still one

          $fromformnumber = optional_param_array('number', '', PARAM_TEXT); 
          

          in edit_calculatedsimple_form.php.

          3. The codeing style is generally very good, but there are still one or two issues that codechecker should find.

          Show
          Tim Hunt added a comment - 1. I am worried by the $ans = $answer->answer; if (strlen($ans) > 20) { $ans = substr($ans, 0, 17).'...'; } part. Why is thix necssary? Surely answers will never be very long and truncating floating point numbers like this seems like a bad idea. (If truncating them was a good idea, you should use the shorten_text function, rather then your own code.) This comment applies in quite a lot of places in the code, where essentially the same thing has been duplicated. 2. There is still one $fromformnumber = optional_param_array('number', '', PARAM_TEXT); in edit_calculatedsimple_form.php. 3. The codeing style is generally very good, but there are still one or two issues that codechecker should find.
          Hide
          Pierre Pichet added a comment - - edited

          Corrections done.
          1. Thanks for the shorten_text() function that I did not know.
          The answer contains the formula whichis tranlated and shown when editing in datasetitem_form (or edit_calculatedsimple_form.

          The following illustrates an "extreme case" display

          Set 1  {x}+{x}+{x}+{x...  123456789+123456789+123456789+123456789+123456789+1234... = 9.88e8
                                    Correct answer : 9.88e8 inside limits of true value 
                                    Min: 977899990.31999 --- Max: 997655545.68001
          

          2. OK
          3. I expect it OK

          Show
          Pierre Pichet added a comment - - edited Corrections done. 1. Thanks for the shorten_text() function that I did not know. The answer contains the formula whichis tranlated and shown when editing in datasetitem_form (or edit_calculatedsimple_form. The following illustrates an "extreme case" display Set 1 {x}+{x}+{x}+{x... 123456789+123456789+123456789+123456789+123456789+1234... = 9.88e8 Correct answer : 9.88e8 inside limits of true value Min: 977899990.31999 --- Max: 997655545.68001 2. OK 3. I expect it OK
          Hide
          Tim Hunt added a comment -

          Thanks Pierre. Submitting for integration.

          Show
          Tim Hunt added a comment - Thanks Pierre. Submitting for integration.
          Hide
          Pierre Pichet added a comment - - edited

          "I think that this could be applied to actual releases i.e 2.3 2.4 2.5 as this does not changed the grading which is done using the exact calculated value, but only the value display as the correct one."
          However the actual "calculated correct response" is within the tolerance of the exact calculated value which could not be the case of the NEW correctly formatted correct response.
          So keep this to 2.6

          Show
          Pierre Pichet added a comment - - edited "I think that this could be applied to actual releases i.e 2.3 2.4 2.5 as this does not changed the grading which is done using the exact calculated value, but only the value display as the correct one." However the actual "calculated correct response" is within the tolerance of the exact calculated value which could not be the case of the NEW correctly formatted correct response. So keep this to 2.6
          Hide
          Tim Lock added a comment -

          Hi Pierre,

          Found a minor issue with your commit #105582781b67510fc1b0f743f24b538c9a3687ba in Moodle 2.3, has conflict markers in it :-

          +<<<<<<< HEAD
          public function multichoice_comment_on_datasetitems($questionid, $questiontext,
          $answers, $data, $number) {
          global $DB;
          @@ -1156,6 +1161,8 @@ class qtype_calculated extends question_type {
          }
          return fullclone($comment);
          }
          +=======
          +>>>>>>> a7bb2a8... MDL-31680 Correcting the format of calculated types correct response

          Show
          Tim Lock added a comment - Hi Pierre, Found a minor issue with your commit #105582781b67510fc1b0f743f24b538c9a3687ba in Moodle 2.3, has conflict markers in it :- +<<<<<<< HEAD public function multichoice_comment_on_datasetitems($questionid, $questiontext, $answers, $data, $number) { global $DB; @@ -1156,6 +1161,8 @@ class qtype_calculated extends question_type { } return fullclone($comment); } +======= +>>>>>>> a7bb2a8... MDL-31680 Correcting the format of calculated types correct response
          Hide
          Dan Poltawski added a comment -

          I'm stopping this as i'd prefer to have Tim about when integrating this change.

          (I can't see Tim Lock's spotted conflict markers though)

          Show
          Dan Poltawski added a comment - I'm stopping this as i'd prefer to have Tim about when integrating this change. (I can't see Tim Lock's spotted conflict markers though)
          Show
          Adam Olley added a comment - Hi Dan, They're in the MDL-31680 _23 branch. https://github.com/ppichet/moodle/commit/105582781b67510fc1b0f743f24b538c9a3687ba
          Hide
          Pierre Pichet added a comment -

          Sorry guys,
          I did not spot earlier your comments.
          The only "valid" version is the MDL-31680 which is the only one referenced in the Tracker
          The other branches on my site were created in the working process.

          See my earlier comment "So keep this to 2.6"

          Show
          Pierre Pichet added a comment - Sorry guys, I did not spot earlier your comments. The only "valid" version is the MDL-31680 which is the only one referenced in the Tracker The other branches on my site were created in the working process. See my earlier comment "So keep this to 2.6"
          Hide
          Dan Poltawski added a comment -

          Integrated to master only.

          Thanks Pierre.

          Show
          Dan Poltawski added a comment - Integrated to master only. Thanks Pierre.
          Hide
          Andrew Davis added a comment -

          This is rather complex but I believe its all working as it has been described. Passing.

          Show
          Andrew Davis added a comment - This is rather complex but I believe its all working as it has been described. Passing.
          Hide
          Dan Poltawski added a comment -

          Thanks for your contributions!

          _main:
          @ BB#0:
                  push    {r7, lr}
                  mov     r7, sp
                  sub     sp, #4
                  movw    r0, :lower16:(L_.str-(LPC0_0+4))
                  movt    r0, :upper16:(L_.str-(LPC0_0+4))
          LPC0_0:
                  add     r0, pc
                  bl      _printf
                  movs    r1, #0
                  movt    r1, #0
                  str     r0, [sp]                @ 4-byte Spill
                  mov     r0, r1
                  add     sp, #4
                  pop     {r7, pc}
          
                  .section        __TEXT,__cstring,cstring_literals
          L_.str:                                 @ @.str
                  .asciz   "This code is now upstream!"
          
          Show
          Dan Poltawski added a comment - Thanks for your contributions! _main: @ BB#0: push {r7, lr} mov r7, sp sub sp, #4 movw r0, :lower16:(L_.str-(LPC0_0+4)) movt r0, :upper16:(L_.str-(LPC0_0+4)) LPC0_0: add r0, pc bl _printf movs r1, #0 movt r1, #0 str r0, [sp] @ 4- byte Spill mov r0, r1 add sp, #4 pop {r7, pc} .section __TEXT,__cstring,cstring_literals L_.str: @ @.str .asciz "This code is now upstream!"

            People

            • Votes:
              4 Vote for this issue
              Watchers:
              11 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: