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

Numerical is_complete() could return a false on a good response

    Details

    • Type: Bug
    • Status: Open
    • Priority: Minor
    • Resolution: Unresolved
    • Affects Version/s: 2.1.6, 2.2.3, 2.3
    • Fix Version/s: None
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide

      The new code allows users to use different numerical formats when creating numerical questions and when answering.
      The format allows to use indifferently , or . as decsep in all language ( i.e. all) where they a used.
      The thousandseps can be the one used in the language setting or space or , or .
      The code is set as follow
      // Dot . is mostly a decimal separator there a few exceptions where it is a thousand separator
      //If a . is present or there are multiple , (i.e. 2,456,789 ) assume
      //is a thousands separator and strip it or else assume it is a decimal
      // separator and change it to . if only one and it is , then change to .

      1.In summary the testing should include creating numerical questions answers with the number formats used in the phpunit test
      as in test_euro_style() as
      -1 000 -1 000 000 -1.000.000 -1,000,000 -1000,100 -1.000,100 -1,000.100 3,14159 .
      Take care that when you create a numerical question either by using edit_numerical_form or as cloze numerical
      the number you write i.e. 10,000.1 WILL BE CONVERTED when saving the question in the STANDARD format i.e. 10000.1
      This flexibility in writing numbers apply for answers and tolerances values of numerical using
      edit_numerical_form or creating numerical using edit_multianswer
      and answering those questions in preview or quiz.

      LOAD THE EXAMPLES from the file
      Set your language to english default
      A. EDIT_NUMERICAL_FORM
      Notice that the answers and tolerance are written as PHP numerical
      Change the values using variants in decsep or thousand sep.
      Save the question and reedit.
      The numerical are converted back to standard PHP.

      Then start a preview of the question and open the reedit so that you can see the answers values and tolerance.

      Preview the question using diffferent response and check if the grading is OK in each case.
      Use the preview "fill the good response " , submit and look at the good response value.

      Verify in the database question_numerical if the tolerance values are stored correctly
      Create a copy and check that everything is OK.

      A. EDIT_MULTIANSWER_FORM

      Modify the values of answers and tolerance and use the
      DECODE AND VERIFY button to check if these values will not set errors.
      Save the variants and preview them using the "fill the good response " to see how these values are stored

      Use different way to write the answers and the tolerance.

      Verify in the database question_numerical if the tolerance values are stored correctly. i.e. 1e2 will be stored as 100

      Verify that the %100%number:tolerance#feedback format could tolerate one space
      between the number:tolerance as

      {1:NUMERICAL:~%100%1,00.1 : 1e2#feedback}

      Create a copy and check that everything is OK

      Testing language.

      Set your language to another where the decsep is , (i.e. français)
      Preview the questions using"fill the good response " , then submit.
      the fill in should ideally replace the decsep by a , in the the good response in the 2 step i.e fill and submit.
      This is true in multianswer but the fill in does not work correctly for numerical with a unit (see the comments below)

      Show
      The new code allows users to use different numerical formats when creating numerical questions and when answering. The format allows to use indifferently , or . as decsep in all language ( i.e. all) where they a used. The thousandseps can be the one used in the language setting or space or , or . The code is set as follow // Dot . is mostly a decimal separator there a few exceptions where it is a thousand separator //If a . is present or there are multiple , (i.e. 2,456,789 ) assume //is a thousands separator and strip it or else assume it is a decimal // separator and change it to . if only one and it is , then change to . 1.In summary the testing should include creating numerical questions answers with the number formats used in the phpunit test as in test_euro_style() as -1 000 -1 000 000 -1.000.000 -1,000,000 -1000,100 -1.000,100 -1,000.100 3,14159 . Take care that when you create a numerical question either by using edit_numerical_form or as cloze numerical the number you write i.e. 10,000.1 WILL BE CONVERTED when saving the question in the STANDARD format i.e. 10000.1 This flexibility in writing numbers apply for answers and tolerances values of numerical using edit_numerical_form or creating numerical using edit_multianswer and answering those questions in preview or quiz. LOAD THE EXAMPLES from the file Set your language to english default A. EDIT_NUMERICAL_FORM Notice that the answers and tolerance are written as PHP numerical Change the values using variants in decsep or thousand sep. Save the question and reedit. The numerical are converted back to standard PHP. Then start a preview of the question and open the reedit so that you can see the answers values and tolerance. Preview the question using diffferent response and check if the grading is OK in each case. Use the preview "fill the good response " , submit and look at the good response value. Verify in the database question_numerical if the tolerance values are stored correctly Create a copy and check that everything is OK. A. EDIT_MULTIANSWER_FORM Modify the values of answers and tolerance and use the DECODE AND VERIFY button to check if these values will not set errors. Save the variants and preview them using the "fill the good response " to see how these values are stored Use different way to write the answers and the tolerance. Verify in the database question_numerical if the tolerance values are stored correctly. i.e. 1e2 will be stored as 100 Verify that the %100%number:tolerance#feedback format could tolerate one space between the number:tolerance as {1:NUMERICAL:~%100%1,00.1 : 1e2#feedback} Create a copy and check that everything is OK Testing language. Set your language to another where the decsep is , (i.e. français) Preview the questions using"fill the good response " , then submit. the fill in should ideally replace the decsep by a , in the the good response in the 2 step i.e fill and submit. This is true in multianswer but the fill in does not work correctly for numerical with a unit (see the comments below)
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      The last step of is_complete_response(array $response) {
      is
      if ($this->ap->contains_thousands_seaparator($response['answer']))

      { return false; }

      This was valid in the first version of the 2,1 new engine apply_units($response, $separateunit = null)
      with its strict syntax analysis.
      However the apply_units() was retrofit to preceeding more error tolerant function() which can handle a , instead of a .
      as decimal point.
      So answering 10,25 for a 10.25 good answer response will generate a incomplete message although on submit and finish the 10,25 will
      grade as 100%.

        Gliffy Diagrams

        1. backup-moodle2-activity-318-quiz318-20130620-1638-nu.mbz
          6 kB
          Frédéric Massart
        2. index1.php.htm
          75 kB
          Pierre Pichet
        3. quiz-T1-EXPORT MDL_33105-20130603-0934.xml
          4 kB
          Pierre Pichet
        1. seperator-hell.png
          44 kB
        2. seperator-hell2.png
          26 kB

          Issue Links

            Activity

            Hide
            ppichet Pierre Pichet added a comment -

            There are different ways to solve this problem.
            I think the most fruitful is to modify the grading process so that the strict apply unit new engine initial design is applied to the response.
            If the grade is not OK then the more flexible apply_units is applied.
            The is_complete_response() should reflect the grading process.

            We should go a step further with an additional grading option that will let the teacher choose between a strict grading as initially designed for the new engine and or the historical more flexible grading.

            Show
            ppichet Pierre Pichet added a comment - There are different ways to solve this problem. I think the most fruitful is to modify the grading process so that the strict apply unit new engine initial design is applied to the response. If the grade is not OK then the more flexible apply_units is applied. The is_complete_response() should reflect the grading process. We should go a step further with an additional grading option that will let the teacher choose between a strict grading as initially designed for the new engine and or the historical more flexible grading.
            Hide
            ppichet Pierre Pichet added a comment -

            An almost universal conversion of number formats with the unique restriction that if you use a thousand separator you MUST add the decsep could (or should ) be the default apply_units ()

            1234.5 1234,5 1 234.5 1,234.5 1.234,5 (deutch) 1(half space)234.5 1(half space)234,5 all grade good.

            I have a first working solution.
            More testing and building unit tests.

            This could be the default and on this we could add a more restrictive grading option using the original new engine code.

            Show
            ppichet Pierre Pichet added a comment - An almost universal conversion of number formats with the unique restriction that if you use a thousand separator you MUST add the decsep could (or should ) be the default apply_units () 1234.5 1234,5 1 234.5 1,234.5 1.234,5 (deutch) 1(half space)234.5 1(half space)234,5 all grade good. I have a first working solution. More testing and building unit tests. This could be the default and on this we could add a more restrictive grading option using the original new engine code.
            Hide
            ppichet Pierre Pichet added a comment -
            Show
            ppichet Pierre Pichet added a comment - A first version is available at https://github.com/ppichet/moodle/tree/MDL-33105 The compare at https://github.com/ppichet/moodle/compare/MDL-33105
            Hide
            ppichet Pierre Pichet added a comment -

            Tim,
            I did not ask for a peer review as the priority was to the 2,3 release.

            However you seem to be back from the mountains in great shape.

            I think that this grading option could be a good and simpler way to fulfill grading in the language number settings.

            This is a first sketch (i.e. without the unit tests).

            Show
            ppichet Pierre Pichet added a comment - Tim, I did not ask for a peer review as the priority was to the 2,3 release. However you seem to be back from the mountains in great shape. I think that this grading option could be a good and simpler way to fulfill grading in the language number settings. This is a first sketch (i.e. without the unit tests).
            Hide
            timhunt Tim Hunt added a comment -

            I am still in the mountains, just doing a few things around the edge of the MoodleMoot. Not sure if I will have time to look at this before I get back or not.

            Show
            timhunt Tim Hunt added a comment - I am still in the mountains, just doing a few things around the edge of the MoodleMoot. Not sure if I will have time to look at this before I get back or not.
            Hide
            ppichet Pierre Pichet added a comment -

            Stay in the mountains as long as you can

            This can wait as long as you want.

            There is no hurry.

            Show
            ppichet Pierre Pichet added a comment - Stay in the mountains as long as you can This can wait as long as you want. There is no hurry.
            Hide
            timhunt Tim Hunt added a comment -

            I just had a quick look while waiting for Martin's keynote to start.

            I am not convinced that this needs to be another option. Can't we just make this work by default? In what ways is the new code not back-wards compatible?

            Show
            timhunt Tim Hunt added a comment - I just had a quick look while waiting for Martin's keynote to start. I am not convinced that this needs to be another option. Can't we just make this work by default? In what ways is the new code not back-wards compatible?
            Hide
            ppichet Pierre Pichet added a comment -

            The actual and historical default is an "almost" universal conversion of numercial formats to PHP . decsep with no thousands sep numerical value.
            This is why you have converted back the apply_units() function and put aside your development with this somehow sad comment
            "* This method can be used for more locale-strict parsing of repsonses. At the

            • moment we don't use it, and instead use the more lax parsing in apply_units.
            • This is just a note that this funciton was used in the past, so if you are
            • intersted, look through version control history.
              "
              The proposal to a grade format penalty was put aside as this give a too complex interface. (MDL-27418_ppichet)

            I just found that it will be easy to offer the choice between a strict conversion or the historical flexible formats (i.e. the necessary default for numerical in cloze numericals).

            So I build this to explore the concept.

            I think that some teachers will like a strict number formating.

            So this is more a proposal than a solution to this bug and others related to numerical formats.

            Anyway, this is already on "I am about to disappear for two weeks"
            http://moodle.org/mod/forum/discuss.php?d=202890

            Show
            ppichet Pierre Pichet added a comment - The actual and historical default is an "almost" universal conversion of numercial formats to PHP . decsep with no thousands sep numerical value. This is why you have converted back the apply_units() function and put aside your development with this somehow sad comment "* This method can be used for more locale-strict parsing of repsonses. At the moment we don't use it, and instead use the more lax parsing in apply_units. This is just a note that this funciton was used in the past, so if you are intersted, look through version control history. " The proposal to a grade format penalty was put aside as this give a too complex interface. ( MDL-27418 _ppichet) I just found that it will be easy to offer the choice between a strict conversion or the historical flexible formats (i.e. the necessary default for numerical in cloze numericals). So I build this to explore the concept. I think that some teachers will like a strict number formating. So this is more a proposal than a solution to this bug and others related to numerical formats. Anyway, this is already on "I am about to disappear for two weeks" http://moodle.org/mod/forum/discuss.php?d=202890
            Hide
            ppichet Pierre Pichet added a comment -

            If we remove the last incomplete test related to the thousand separator we have a temporary solution for MDL-33744 that will solve the most current number formatting as a large number of languages use , as a decsep.
            This eliminate a warning that most often but not in all cases will not modify the grade.
            OR
            We eliminate this test only to cloze numerical which is not a good way to code multianswer...

            We could add the proposed solution for MDL-33105 later.

            Show
            ppichet Pierre Pichet added a comment - If we remove the last incomplete test related to the thousand separator we have a temporary solution for MDL-33744 that will solve the most current number formatting as a large number of languages use , as a decsep. This eliminate a warning that most often but not in all cases will not modify the grade. OR We eliminate this test only to cloze numerical which is not a good way to code multianswer... We could add the proposed solution for MDL-33105 later.
            Hide
            ppichet Pierre Pichet added a comment -

            There are two occurences of
            if ($this->ap->contains_thousands_seaparator($response['answer']))

            in question/type/numerical/question.php
            One in the function is_complete test() and one in the function get_validation_error(array $response)

            They are changed as comment in the "experimental" proposal see

            https://github.com/ppichet/moodle/compare/MDL-33105

            Show
            ppichet Pierre Pichet added a comment - There are two occurences of if ($this->ap->contains_thousands_seaparator($response ['answer'] )) in question/type/numerical/question.php One in the function is_complete test() and one in the function get_validation_error(array $response) They are changed as comment in the "experimental" proposal see https://github.com/ppichet/moodle/compare/MDL-33105
            Hide
            timhunt Tim Hunt added a comment -

            I finally got back to looking at this.

            I like your original proposal here: http://tracker.moodle.org/browse/MDL-33105?focusedCommentId=158563&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-158563 which does not require any question options.

            It seems to me that will just work in almost all cases.

            Therefore, I don't think we need the complexity of adding new options.

            Show
            timhunt Tim Hunt added a comment - I finally got back to looking at this. I like your original proposal here: http://tracker.moodle.org/browse/MDL-33105?focusedCommentId=158563&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-158563 which does not require any question options. It seems to me that will just work in almost all cases. Therefore, I don't think we need the complexity of adding new options.
            Hide
            ppichet Pierre Pichet added a comment -

            Unless there is a specific need, I put this on my todo list (back from Collioure ...)

            Show
            ppichet Pierre Pichet added a comment - Unless there is a specific need, I put this on my todo list (back from Collioure ...)
            Hide
            ppichet Pierre Pichet added a comment -

            So back to this as this is related to a "critical" MDL-33744 and it seems that I can put some time this week on moodle coding.
            So as you suggest, I will test the more flexible apply_unit() function as described in https://github.com/ppichet/moodle/compare/MDL-33105
            without the new question numberdecodingtype option.

            Show
            ppichet Pierre Pichet added a comment - So back to this as this is related to a "critical" MDL-33744 and it seems that I can put some time this week on moodle coding. So as you suggest, I will test the more flexible apply_unit() function as described in https://github.com/ppichet/moodle/compare/MDL-33105 without the new question numberdecodingtype option.
            Hide
            ppichet Pierre Pichet added a comment -

            The github has been updated to a simpler improved apply_units().
            The other (options) solution being stored (just in case) as https://github.com/ppichet/moodle/tree/MDL-33105_strictformat
            I am begining testing and building the appropriate testunits..

            Show
            ppichet Pierre Pichet added a comment - The github has been updated to a simpler improved apply_units(). The other (options) solution being stored (just in case) as https://github.com/ppichet/moodle/tree/MDL-33105_strictformat I am begining testing and building the appropriate testunits..
            Hide
            ppichet Pierre Pichet added a comment -

            As the teacher is not able to set for a strict format but want that the thousand sep could be used, this imply that
            for an ambiguous entry we choose the more general one.
            The use of . for thousand sep in deutsch language cannot be distinguished from the english decimal sep unless there is a , after the . as the fourth character i.e. 12.345,67 and furthermore that the entry in done when deutsch is set as the language.

            However 12.345.678 is unambiguous as 12,345,678 as 12 345 678

            So I go back to the drawing board

            Show
            ppichet Pierre Pichet added a comment - As the teacher is not able to set for a strict format but want that the thousand sep could be used, this imply that for an ambiguous entry we choose the more general one. The use of . for thousand sep in deutsch language cannot be distinguished from the english decimal sep unless there is a , after the . as the fourth character i.e. 12.345,67 and furthermore that the entry in done when deutsch is set as the language. However 12.345.678 is unambiguous as 12,345,678 as 12 345 678 So I go back to the drawing board
            Hide
            ppichet Pierre Pichet added a comment -

            Tim,
            When you built the new question engine, your first reaction was to build a strict format for numbers in your qtype_numerical_answer_processor().
            Because of the Moodle history, you put this handling aside.
            From other english Moodle users comments, your personal "strict" attitude seems quite normal.
            Other Moodle users from other countries express their need to have the possibility of grading their own number format.

            So I will build the "almost" universal numerical format reading without removing the possibility to a future number format grading.

            Incidently almost all languages use the indoeuropean numerical with the same numbers order i.e.

            "In Arabic multidigit numbers are written right-to-left, in increasing powers of ten. Thus, just as in the west, the units are in the rightmost position and greater powers of ten are to the left"

            http://en.wikipedia.org/wiki/List_of_numbers_in_various_languages#Indo-European_languages

            Show
            ppichet Pierre Pichet added a comment - Tim, When you built the new question engine, your first reaction was to build a strict format for numbers in your qtype_numerical_answer_processor(). Because of the Moodle history, you put this handling aside. From other english Moodle users comments, your personal "strict" attitude seems quite normal. Other Moodle users from other countries express their need to have the possibility of grading their own number format. So I will build the "almost" universal numerical format reading without removing the possibility to a future number format grading. Incidently almost all languages use the indoeuropean numerical with the same numbers order i.e. "In Arabic multidigit numbers are written right-to-left, in increasing powers of ten. Thus, just as in the west, the units are in the rightmost position and greater powers of ten are to the left" http://en.wikipedia.org/wiki/List_of_numbers_in_various_languages#Indo-European_languages
            Hide
            timhunt Tim Hunt added a comment -

            What I would most like to see is an update to https://github.com/ppichet/moodle/blob/71c238b5494dcd7431ee25893c8869a81e235490/question/type/numerical/tests/answerprocessor_test.php so it is easy to see at a glance lots of examples of how the new code will work.

            Show
            timhunt Tim Hunt added a comment - What I would most like to see is an update to https://github.com/ppichet/moodle/blob/71c238b5494dcd7431ee25893c8869a81e235490/question/type/numerical/tests/answerprocessor_test.php so it is easy to see at a glance lots of examples of how the new code will work.
            Hide
            ppichet Pierre Pichet added a comment -

            This is also my next step.
            I am completing the phpunit installation ( as usual the docs are not always up-to-date ) and I will give you the info as soon as possible.
            let's say that my "manual" tests
            10,3 10.3 10,300.0e-3 10.300,0e-3 10300.0e-3 10300,0e-3 ... give the same grade either in english, french and deustch set as language

            Show
            ppichet Pierre Pichet added a comment - This is also my next step. I am completing the phpunit installation ( as usual the docs are not always up-to-date ) and I will give you the info as soon as possible. let's say that my "manual" tests 10,3 10.3 10,300.0e-3 10.300,0e-3 10300.0e-3 10300,0e-3 ... give the same grade either in english, french and deustch set as language
            Hide
            ppichet Pierre Pichet added a comment -

            AS far as I can understand my PHPUnit installation problems seems to be related to having followed
            http://docs.moodle.org/dev/PHPUnit_installation_in_Windows#Install_PHPUnit
            and did not noticed
            http://docs.moodle.org/dev/PHPUnit#Installation_of_PHPUnit_via_PEAR_.28not_recommended.29
            As I worked remotely on my desktop computer, more news next week...

            Show
            ppichet Pierre Pichet added a comment - AS far as I can understand my PHPUnit installation problems seems to be related to having followed http://docs.moodle.org/dev/PHPUnit_installation_in_Windows#Install_PHPUnit and did not noticed http://docs.moodle.org/dev/PHPUnit#Installation_of_PHPUnit_via_PEAR_.28not_recommended.29 As I worked remotely on my desktop computer, more news next week...
            Hide
            timhunt Tim Hunt added a comment -

            Actually, you did not make a mistake. The Composer stuff is very new. It has only been there for about a week. It is much easier than the old way, which is why they changed.

            Show
            timhunt Tim Hunt added a comment - Actually, you did not make a mistake. The Composer stuff is very new. It has only been there for about a week. It is much easier than the old way, which is why they changed.
            Hide
            ppichet Pierre Pichet added a comment - - edited

            Just to tell that I don't give up to install PHPUnit but the learning curve is steep.
            As an example I just finally found what vendor means in
            "To execute all test suites from main configuration file execute the vendor/bin/phpunit script from your $CFG->dirroot directory."

            I have never went so far in the intricacies of the Apache-Windows commands.
            Finally the "vendor" was set by the installer to something as evident as
            C:\Documents and Settings\All Users\Application Data\Composer\

            Show
            ppichet Pierre Pichet added a comment - - edited Just to tell that I don't give up to install PHPUnit but the learning curve is steep. As an example I just finally found what vendor means in "To execute all test suites from main configuration file execute the vendor/bin/phpunit script from your $CFG->dirroot directory." I have never went so far in the intricacies of the Apache-Windows commands. Finally the "vendor" was set by the installer to something as evident as C:\Documents and Settings\All Users\Application Data\Composer\
            Hide
            ppichet Pierre Pichet added a comment -

            Tim,
            The Apache path seems to be Ok because if I go to my moodle dirroot as D:moodle2\server\moodle and type Composer
            I obtain a list of
            Usage:
            Options
            Available commands:
            etc.

            However from there I don't know how proceed to install PHPUnit from http://docs.moodle.org/dev/PHPUnit or http://docs.moodle.org/dev/PHPUnit_installation_in_Windows or the Composer docs.

            Show
            ppichet Pierre Pichet added a comment - Tim, The Apache path seems to be Ok because if I go to my moodle dirroot as D:moodle2\server\moodle and type Composer I obtain a list of Usage: Options Available commands: etc. However from there I don't know how proceed to install PHPUnit from http://docs.moodle.org/dev/PHPUnit or http://docs.moodle.org/dev/PHPUnit_installation_in_Windows or the Composer docs.
            Hide
            timhunt Tim Hunt added a comment -

            It should be as simple as http://docs.moodle.org/dev/PHPUnit#Installation_of_PHPUnit_via_Composer

            Except that that refers to Unix commands like curl. Does http://getcomposer.org/doc/00-intro.md#installation-windows help?

            I am not at work (where I have a Windows computer) today. Tomorrow, I will try installing PHPUnit using Composer, and produce some clear documentation.

            Show
            timhunt Tim Hunt added a comment - It should be as simple as http://docs.moodle.org/dev/PHPUnit#Installation_of_PHPUnit_via_Composer Except that that refers to Unix commands like curl. Does http://getcomposer.org/doc/00-intro.md#installation-windows help? I am not at work (where I have a Windows computer) today. Tomorrow, I will try installing PHPUnit using Composer, and produce some clear documentation.
            Hide
            ppichet Pierre Pichet added a comment - - edited

            just find
            http://www.phpunit.de/manual/current/en/installation.html#installation.composer
            will try and give news.
            To summarize
            After experimenting with PEAR which effectively load somehow PHPUnit code that did not seem to work, I try the windows installer from http://getcomposer.org/doc/00-intro.md
            "Download and run Composer-Setup.exe, it will install the latest Composer version and set up your PATH so that you can just call composer from any directory in your command line."

            This install the composer in
            C:\Documents and Settings\All Users\Application Data\Composer\bin
            then I did
            C:\Documents and Settings\All Users\Application Data\Composer\bin>Composer install
            Loading composer repositories with package information
            Installing dependencies
            Nothing to install or update
            Generating autoload files

            This create the directories vendor and composer i.e.
            C:\Documents and Settings\All Users\Application Data\Composer\bin\vendor\composer

            Probably bacause of the multiple installations, things are not working properly.
            I will reinstall everything clean once you have clarified the correct procedure.

            Show
            ppichet Pierre Pichet added a comment - - edited just find http://www.phpunit.de/manual/current/en/installation.html#installation.composer will try and give news. To summarize After experimenting with PEAR which effectively load somehow PHPUnit code that did not seem to work, I try the windows installer from http://getcomposer.org/doc/00-intro.md "Download and run Composer-Setup.exe, it will install the latest Composer version and set up your PATH so that you can just call composer from any directory in your command line." This install the composer in C:\Documents and Settings\All Users\Application Data\Composer\bin then I did C:\Documents and Settings\All Users\Application Data\Composer\bin>Composer install Loading composer repositories with package information Installing dependencies Nothing to install or update Generating autoload files This create the directories vendor and composer i.e. C:\Documents and Settings\All Users\Application Data\Composer\bin\vendor\composer Probably bacause of the multiple installations, things are not working properly. I will reinstall everything clean once you have clarified the correct procedure.
            Hide
            ppichet Pierre Pichet added a comment -

            I miss the --dev in Install
            Was added 3 days ago in the doc.
            So I just try it

            C:\Documents and Settings\All Users\Application Data\Composer\bin>Composer insta
            ll --dev
            Loading composer repositories with package information
            Installing dependencies
            Nothing to install or update
            Loading composer repositories with package information
            Installing dev dependencies

            • Installing symfony/yaml (v2.1.4)
              Downloading: 100%
            • Installing phpunit/php-text-template (1.1.4)
              Downloading: 100%
            • Installing phpunit/phpunit-mock-objects (1.2.2)
              Downloading: 100%
            • Installing phpunit/php-timer (1.0.4)
              Downloading: 100%
            • Installing phpunit/php-token-stream (1.1.5)
              Downloading: 100%
            • Installing phpunit/php-file-iterator (1.3.3)
              Downloading: 100%
            • Installing phpunit/php-code-coverage (1.2.7)
              Downloading: 100%
            • Installing phpunit/phpunit (3.7.10)
              Downloading: 100%
            • Installing phpunit/dbunit (1.2.1)
              Downloading: 100%

            phpunit/php-code-coverage suggests installing ext-xdebug (>=2.0.5)
            phpunit/phpunit suggests installing phpunit/php-invoker (>=1.1.0)
            Writing lock file
            Generating autoload files

            Continuing the testing later today

            Show
            ppichet Pierre Pichet added a comment - I miss the --dev in Install Was added 3 days ago in the doc. So I just try it C:\Documents and Settings\All Users\Application Data\Composer\bin>Composer insta ll --dev Loading composer repositories with package information Installing dependencies Nothing to install or update Loading composer repositories with package information Installing dev dependencies Installing symfony/yaml (v2.1.4) Downloading: 100% Installing phpunit/php-text-template (1.1.4) Downloading: 100% Installing phpunit/phpunit-mock-objects (1.2.2) Downloading: 100% Installing phpunit/php-timer (1.0.4) Downloading: 100% Installing phpunit/php-token-stream (1.1.5) Downloading: 100% Installing phpunit/php-file-iterator (1.3.3) Downloading: 100% Installing phpunit/php-code-coverage (1.2.7) Downloading: 100% Installing phpunit/phpunit (3.7.10) Downloading: 100% Installing phpunit/dbunit (1.2.1) Downloading: 100% phpunit/php-code-coverage suggests installing ext-xdebug (>=2.0.5) phpunit/phpunit suggests installing phpunit/php-invoker (>=1.1.0) Writing lock file Generating autoload files Continuing the testing later today
            Hide
            timhunt Tim Hunt added a comment -

            Show
            timhunt Tim Hunt added a comment -
            Hide
            ppichet Pierre Pichet added a comment - - edited

            Probably because of my weird installation (see the phpinfo (index1.php.htm)) I don't know how to call phpunit

            C:\Documents and Settings\All Users\Application Data\Composer\bin\vendor\phpunit\phpunit\phpunit.php

            Note that composer is recognized at the location of the http://132.208.141.198/moodle_w/ site i.e.
            D:/moodle2/server/moodle/moodle_w

            Show
            ppichet Pierre Pichet added a comment - - edited Probably because of my weird installation (see the phpinfo (index1.php.htm)) I don't know how to call phpunit C:\Documents and Settings\All Users\Application Data\Composer\bin\vendor\phpunit\phpunit\phpunit.php Note that composer is recognized at the location of the http://132.208.141.198/moodle_w/ site i.e. D:/moodle2/server/moodle/moodle_w
            Hide
            ppichet Pierre Pichet added a comment - - edited

            Tim,
            Its working ...

            Finally just get and put at the right place the correct instructions with the correct / or \ ...

            Si I passed the test

            Show
            ppichet Pierre Pichet added a comment - - edited Tim, Its working ... Finally just get and put at the right place the correct instructions with the correct / or \ ... Si I passed the test
            Hide
            timhunt Tim Hunt added a comment -

            Great! I am glad you could work it out. Sorry I was too busy to help.

            Show
            timhunt Tim Hunt added a comment - Great! I am glad you could work it out. Sorry I was too busy to help.
            Hide
            ppichet Pierre Pichet added a comment - - edited

            Tim,
            Do I understand well that the main phpunit test that I need to modify is

            public function test_euro_style() {
                    $ap = new qtype_numerical_answer_processor(array(), false, ',', ' ');
             
                    $this->assertEquals(array(-1000, '', null), $ap->apply_units('-1 000'));
                    $this->assertEquals(array(3.14159, '', null), $ap->apply_units('3,14159'));
                }
            

            inside numerical/tests/answerprocessor_test.php
            I should also modify the is_complete_response(array $response) { test as we will drop the last step

            if ($this->ap->contains_thousands_seaparator($response['answer'])) {

            However this was apparently not tested

             
            public function test_is_complete_response() {
                    $question = test_question_maker::make_question('numerical');
             
                    $this->assertFalse($question->is_complete_response(array()));
                    $this->assertTrue($question->is_complete_response(array('answer' => '0')));
                    $this->assertTrue($question->is_complete_response(array('answer' => 0)));
                    $this->assertFalse($question->is_complete_response(array('answer' => 'test')));
                }
            

            Show
            ppichet Pierre Pichet added a comment - - edited Tim, Do I understand well that the main phpunit test that I need to modify is public function test_euro_style() { $ap = new qtype_numerical_answer_processor(array(), false, ',', ' ');   $this->assertEquals(array(-1000, '', null), $ap->apply_units('-1 000')); $this->assertEquals(array(3.14159, '', null), $ap->apply_units('3,14159')); } inside numerical/tests/answerprocessor_test.php I should also modify the is_complete_response(array $response) { test as we will drop the last step if ($this->ap->contains_thousands_seaparator($response ['answer'] )) { However this was apparently not tested   public function test_is_complete_response() { $question = test_question_maker::make_question('numerical');   $this->assertFalse($question->is_complete_response(array())); $this->assertTrue($question->is_complete_response(array('answer' => '0'))); $this->assertTrue($question->is_complete_response(array('answer' => 0))); $this->assertFalse($question->is_complete_response(array('answer' => 'test'))); }
            Hide
            timhunt Tim Hunt added a comment -

            I think you are correct about which tests need to be modified.

            Show
            timhunt Tim Hunt added a comment - I think you are correct about which tests need to be modified.
            Hide
            ppichet Pierre Pichet added a comment -

            The following tests are OK

             
                public function test_euro_style() {
                    $ap = new qtype_numerical_answer_processor(array(), false, ',', ' ');
             
                    $this->assertEquals(array(-1000, '', null), $ap->apply_units('-1 000'));
                    $this->assertEquals(array(-1000000, '', null), $ap->apply_units('-1 000 000'));
                    $this->assertEquals(array(-1000000, '', null), $ap->apply_units('-1.000.000'));
                    $this->assertEquals(array(-1000000, '', null), $ap->apply_units('-1,000,000'));
                    $this->assertEquals(array(-1000.1, '', null), $ap->apply_units('-1000,1'));
                    $this->assertEquals(array(-1000.1, '', null), $ap->apply_units('-1.000,1'));
                    $this->assertEquals(array(-1000.1, '', null), $ap->apply_units('-1,000.1'));
                    $this->assertEquals(array(3.14159, '', null), $ap->apply_units('3,14159'));
                }

            The space in 1 000 000 is officially not a regular space but a non-breaking space.
            http://en.wikipedia.org/wiki/Non-breaking_space
            However as the response will be typed in a regular input element, I don't think that the normal user will use the keys combination (...) that will be equivalent to this non-breaking space.

            Show
            ppichet Pierre Pichet added a comment - The following tests are OK public function test_euro_style() { $ap = new qtype_numerical_answer_processor(array(), false, ',', ' ');   $this->assertEquals(array(-1000, '', null), $ap->apply_units('-1 000')); $this->assertEquals(array(-1000000, '', null), $ap->apply_units('-1 000 000')); $this->assertEquals(array(-1000000, '', null), $ap->apply_units('-1.000.000')); $this->assertEquals(array(-1000000, '', null), $ap->apply_units('-1,000,000')); $this->assertEquals(array(-1000.1, '', null), $ap->apply_units('-1000,1')); $this->assertEquals(array(-1000.1, '', null), $ap->apply_units('-1.000,1')); $this->assertEquals(array(-1000.1, '', null), $ap->apply_units('-1,000.1')); $this->assertEquals(array(3.14159, '', null), $ap->apply_units('3,14159')); } The space in 1 000 000 is officially not a regular space but a non-breaking space. http://en.wikipedia.org/wiki/Non-breaking_space However as the response will be typed in a regular input element, I don't think that the normal user will use the keys combination (...) that will be equivalent to this non-breaking space.
            Hide
            ppichet Pierre Pichet added a comment - - edited

            Adding a specific test_deutsch_style() function to test . as thousand separator and , as decsep.
            The logic used is as long as the deustch user use a single . as thousand separator ( i.e. 1.000,1) he MUST simultanously use the , decsep so the code can identify the . as a thousand separator.
            With larger numbers which necessitate more than 1 thousand separator (i.e 1 million 1.000.000 ) the decsep is not necessary.

            Show
            ppichet Pierre Pichet added a comment - - edited Adding a specific test_deutsch_style() function to test . as thousand separator and , as decsep. The logic used is as long as the deustch user use a single . as thousand separator ( i.e. 1.000,1) he MUST simultanously use the , decsep so the code can identify the . as a thousand separator. With larger numbers which necessitate more than 1 thousand separator (i.e 1 million 1.000.000 ) the decsep is not necessary.
            Hide
            ppichet Pierre Pichet added a comment -

            The phpunit tests seem OK.

            Show
            ppichet Pierre Pichet added a comment - The phpunit tests seem OK.
            Hide
            ppichet Pierre Pichet added a comment -

            This works correctly with numerical but not when editing with cloze although it is Ok when answering the question (response 1.000,1 is correctly interpreted as 1000.1 )
            So I will get back to the drawing board and take a look to the cloze edit form validation.
            Will also test calculateds.

            Show
            ppichet Pierre Pichet added a comment - This works correctly with numerical but not when editing with cloze although it is Ok when answering the question (response 1.000,1 is correctly interpreted as 1000.1 ) So I will get back to the drawing board and take a look to the cloze edit form validation. Will also test calculateds.
            Hide
            ppichet Pierre Pichet added a comment -

            The test done in cloze is the following.

            if ($subquestion->qtype == 'numerical' &&
                                                    !(is_numeric($trimmedanswer) || $trimmedanswer == '*')) {
                                                $this->_form->setElementError($prefix.'answer['.$key.']',
                                                        get_string('answermustbenumberorstar',
                                                                'qtype_numerical'));
                                            }

            this is another bug MDL-29691 so it should solved separately .

            Show
            ppichet Pierre Pichet added a comment - The test done in cloze is the following. if ($subquestion->qtype == 'numerical' && !(is_numeric($trimmedanswer) || $trimmedanswer == '*')) { $this->_form->setElementError($prefix.'answer['.$key.']', get_string('answermustbenumberorstar', 'qtype_numerical')); } this is another bug MDL-29691 so it should solved separately .
            Hide
            ppichet Pierre Pichet added a comment -

            For calculated simple the formula being pure php code does not tolerate thousandseps or decsep other than .
            The response however as for the cloze is interpreted with the new code so 1.000,1 is given the same 100% grade as 1000.1

            Things seems OK for further testing by someone else than me.

            Show
            ppichet Pierre Pichet added a comment - For calculated simple the formula being pure php code does not tolerate thousandseps or decsep other than . The response however as for the cloze is interpreted with the new code so 1.000,1 is given the same 100% grade as 1000.1 Things seems OK for further testing by someone else than me.
            Hide
            ppichet Pierre Pichet added a comment - - edited

            We should note that this solution is user-friendly in a specific way.
            When editing the question the user can write the answer as 1.000,1.
            The answer saved is the correct php integer format i.e. 1000.1
            The student can write 1.000,1 as answer but the correct answer in numerical shown will be 1000.1 or 1000,1 following the language setting decsep.
            However the correct answer is not decsep compliant in multianswer rendering.

            As the thousand seps were not allowed, they are not implemented in the correct answer rendering.

            Show
            ppichet Pierre Pichet added a comment - - edited We should note that this solution is user-friendly in a specific way. When editing the question the user can write the answer as 1.000,1. The answer saved is the correct php integer format i.e. 1000.1 The student can write 1.000,1 as answer but the correct answer in numerical shown will be 1000.1 or 1000,1 following the language setting decsep. However the correct answer is not decsep compliant in multianswer rendering. As the thousand seps were not allowed, they are not implemented in the correct answer rendering.
            Hide
            ppichet Pierre Pichet added a comment - - edited

            "However the correct answer is not decsep compliant in multianswer rendering."
            This is related to the not correct call in multianswer rendering

            if ($subq->qtype->name() == 'shortanswer') {
                        $correctanswer = $subq->get_matching_answer($subq->get_correct_response());
                    } else {
                        $correctanswer = $subq->get_correct_answer();
                    }
            

            in 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);
                }
            

            which cannot be used as is given

                    $feedbackpopup = $this->feedback_popup($subq, $matchinganswer->fraction,
                            $subq->format_text($matchinganswer->feedback, $matchinganswer->feedbackformat,
                                    $qa, 'question', 'answerfeedback', $matchinganswer->id),
                            s($correctanswer->answer), $options);
            
            

            The simplest solution will be to copy in multianswer the part of the code that write the decsep

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

            I will try and give the result.

            Show
            ppichet Pierre Pichet added a comment - - edited "However the correct answer is not decsep compliant in multianswer rendering." This is related to the not correct call in multianswer rendering if ($subq->qtype->name() == 'shortanswer') { $correctanswer = $subq->get_matching_answer($subq->get_correct_response()); } else { $correctanswer = $subq->get_correct_answer(); } in 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); } which cannot be used as is given $feedbackpopup = $this->feedback_popup($subq, $matchinganswer->fraction, $subq->format_text($matchinganswer->feedback, $matchinganswer->feedbackformat, $qa, 'question', 'answerfeedback', $matchinganswer->id), s($correctanswer->answer), $options); The simplest solution will be to copy in multianswer the part of the code that write the decsep $response = str_replace('.', $question->ap->get_point(), $answer->answer); I will try and give the result.
            Hide
            ppichet Pierre Pichet added a comment -

            If you agree with the logic, this is ready to go ...

            Show
            ppichet Pierre Pichet added a comment - If you agree with the logic, this is ready to go ...
            Hide
            timhunt Tim Hunt added a comment -

            You definitely need to run codechecker (https://github.com/moodlehq/moodle-local_codechecker). The integrators will reject the patch as it is.

            Apart from that, this change looks good.

            I still worry that an OU accounting student who types £1,000 will have that treated as £1, but that is the only case that is handled wrongly, and I cannot see any way around that, so I think we have to live with it.

            Show
            timhunt Tim Hunt added a comment - You definitely need to run codechecker ( https://github.com/moodlehq/moodle-local_codechecker ). The integrators will reject the patch as it is. Apart from that, this change looks good. I still worry that an OU accounting student who types £1,000 will have that treated as £1, but that is the only case that is handled wrongly, and I cannot see any way around that, so I think we have to live with it.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment - - edited

            Using commas as thousand separator is evil ! it should be punished by law In fact using anything other than nothing or space is evil !

            Show
            jmvedrine Jean-Michel Vedrine added a comment - - edited Using commas as thousand separator is evil ! it should be punished by law In fact using anything other than nothing or space is evil !
            Hide
            timhunt Tim Hunt added a comment -

            OK, so looking in our live database, wo have 211712 responses to numerical questions.

            (It seems that the most response is 5, given 7255 times.)

            I see about 115 responses that are like 445,000, where ',' is almost certainly being used as a thousand separator.

            I also see many more rows where people are writing decimal numbers with a ',' as decimal point. This includes some like '0,821' which, becuase of the leading 0, is clearly a decimal even though it has three digits after the ','.

            So, on the whole, I think this change is OK.

            Show
            timhunt Tim Hunt added a comment - OK, so looking in our live database, wo have 211712 responses to numerical questions. (It seems that the most response is 5, given 7255 times.) I see about 115 responses that are like 445,000, where ',' is almost certainly being used as a thousand separator. I also see many more rows where people are writing decimal numbers with a ',' as decimal point. This includes some like '0,821' which, becuase of the leading 0, is clearly a decimal even though it has three digits after the ','. So, on the whole, I think this change is OK.
            Hide
            timhunt Tim Hunt added a comment -

            SELECT qasd.value, COUNT(1) AS frequency
            FROM mdl_question_attempt_step_data qasd
            JOIN mdl_question_attempt_steps qas ON qas.id = qasd.attemptstepid
            JOIN mdl_question_attempts qa ON qa.id = qas.questionattemptid
            JOIN mdl_question q ON q.id = qa.questionid
            WHERE q.qtype = 'numerical'
            AND qasd.name = 'answer'
            GROUP BY qasd.value
            ORDER BY frequency DESC

            if you want to try this on your own database.

            Show
            timhunt Tim Hunt added a comment - SELECT qasd.value, COUNT(1) AS frequency FROM mdl_question_attempt_step_data qasd JOIN mdl_question_attempt_steps qas ON qas.id = qasd.attemptstepid JOIN mdl_question_attempts qa ON qa.id = qas.questionattemptid JOIN mdl_question q ON q.id = qa.questionid WHERE q.qtype = 'numerical' AND qasd.name = 'answer' GROUP BY qasd.value ORDER BY frequency DESC if you want to try this on your own database.
            Hide
            ppichet Pierre Pichet added a comment -

            I will install the code checker and work on this not until friday.

            Show
            ppichet Pierre Pichet added a comment - I will install the code checker and work on this not until friday.
            Hide
            ppichet Pierre Pichet added a comment -

            "I still worry that an OU accounting student who types £1,000 will have that treated as £1, but that is the only case that is handled wrongly, and I cannot see any way around that, so I think we have to live with it."

            The student will not be incited to write the thousand separator as the correct response will not show any thousand separator.
            The correct answer is the number as written by php following a math operation so without any formatting apart the decsep or the E .

            Show
            ppichet Pierre Pichet added a comment - "I still worry that an OU accounting student who types £1,000 will have that treated as £1, but that is the only case that is handled wrongly, and I cannot see any way around that, so I think we have to live with it." The student will not be incited to write the thousand separator as the correct response will not show any thousand separator. The correct answer is the number as written by php following a math operation so without any formatting apart the decsep or the E .
            Hide
            ppichet Pierre Pichet added a comment -

            I finally got some time and pass the three files on code checker.
            for tests/answerprocessor_test.phph I got only the warning
            ····public·function·parse_response($response)·{
            31: Useless method overriding detected

            I pass again the phpunit for the numerical answerprocessor_text and everything remains OK

            I push the new version on GitHub.

            So it seems ready to tests.

            As in this code we eliminate the incomplete answer test related to the use of thousands seps, it could be useful
            to push to master this one first before finishing MDL-29691 which will use the apply_units function in multianswer.

            In all cases, what is the next step ?

            Show
            ppichet Pierre Pichet added a comment - I finally got some time and pass the three files on code checker. for tests/answerprocessor_test.phph I got only the warning ····public·function·parse_response($response)·{ 31: Useless method overriding detected I pass again the phpunit for the numerical answerprocessor_text and everything remains OK I push the new version on GitHub. So it seems ready to tests. As in this code we eliminate the incomplete answer test related to the use of thousands seps, it could be useful to push to master this one first before finishing MDL-29691 which will use the apply_units function in multianswer. In all cases, what is the next step ?
            Hide
            timhunt Tim Hunt added a comment -

            The "Useless method overriding detected" message is a common false-positive from codechecker. Since it is only checking one file at a time, in cannot see that the overriding is useful: It changes protected in the base class to public.

            The next step is that I review the code. ... Done.

            Then, if I am happy, I submit it for integration.

            Ah! one more thing is missing before I can submit this for integration. You need to write some "Testing Instructions" to explain to someone how to test this feature. Can you do that?

            Show
            timhunt Tim Hunt added a comment - The "Useless method overriding detected" message is a common false-positive from codechecker. Since it is only checking one file at a time, in cannot see that the overriding is useful: It changes protected in the base class to public. The next step is that I review the code. ... Done. Then, if I am happy, I submit it for integration. Ah! one more thing is missing before I can submit this for integration. You need to write some "Testing Instructions" to explain to someone how to test this feature. Can you do that?
            Hide
            ppichet Pierre Pichet added a comment -

            I updated the testing instruction field.

            Show
            ppichet Pierre Pichet added a comment - I updated the testing instruction field.
            Hide
            poltawski Dan Poltawski added a comment -

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

            TIA and ciao

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

            I rebased MDL-29691 although the process does not work easily and trying to rebase MDL-33105 these problems continue.
            I try it verbose and it stop somewhere in the process (create mode ...)

            Can I do something similar to rebase using cherry-pick ?

            Show
            ppichet Pierre Pichet added a comment - I rebased MDL-29691 although the process does not work easily and trying to rebase MDL-33105 these problems continue. I try it verbose and it stop somewhere in the process (create mode ...) Can I do something similar to rebase using cherry-pick ?
            Hide
            ppichet Pierre Pichet added a comment -

            I did a cherry-pick and this seems to work ?

            https://github.com/ppichet/moodle/compare/MDL-33105

            Show
            ppichet Pierre Pichet added a comment - I did a cherry-pick and this seems to work ? https://github.com/ppichet/moodle/compare/MDL-33105
            Hide
            timhunt Tim Hunt added a comment -

            That looks correct. Thanks. I don't know why rebase did not work.

            Show
            timhunt Tim Hunt added a comment - That looks correct. Thanks. I don't know why rebase did not work.
            Hide
            ppichet Pierre Pichet added a comment -

            If we look at the last version of GIT for developper.
            http://docs.moodle.org/dev/Git_for_developers#Applying_changes_from_one_branch_to_another
            Given that we put the modifications in a single commit, cherry-picking appeared to me equivalent to rebasing.
            I effectively applied the old master commit to the new master as I would apply it to another Moodle version.

            Show
            ppichet Pierre Pichet added a comment - If we look at the last version of GIT for developper. http://docs.moodle.org/dev/Git_for_developers#Applying_changes_from_one_branch_to_another Given that we put the modifications in a single commit, cherry-picking appeared to me equivalent to rebasing. I effectively applied the old master commit to the new master as I would apply it to another Moodle version.
            Hide
            timhunt Tim Hunt added a comment -

            Indeed. Cherry-pick and rebase are two different ways to specify very similar operations. (For a given situation, one is usually more natural than the other.)

            Show
            timhunt Tim Hunt added a comment - Indeed. Cherry-pick and rebase are two different ways to specify very similar operations. (For a given situation, one is usually more natural than the other.)
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Hi, any reason this should go to 2.5 only? Or is it back-porable to 2.4 too?

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Hi, any reason this should go to 2.5 only? Or is it back-porable to 2.4 too?
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            ping?

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - ping?
            Hide
            timhunt Tim Hunt added a comment -

            Sorry, I thought I replied to this. This one should not be back-ported. It changes the behaviour in a few edge cases in a way that I don't think is suitable for stable branches.

            Show
            timhunt Tim Hunt added a comment - Sorry, I thought I replied to this. This one should not be back-ported. It changes the behaviour in a few edge cases in a way that I don't think is suitable for stable branches.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            FYI: It's Wednesday and I'm moving all MY "integration in progress" issues (3) out from current integration with message:

            The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.

            This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.

            This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P

            Apologies for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - FYI: It's Wednesday and I'm moving all MY "integration in progress" issues (3) out from current integration with message: The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologies for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

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

            TIA and ciao

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

            Integrated to master only. Thanks Pierre.

            Show
            poltawski Dan Poltawski added a comment - Integrated to master only. Thanks Pierre.
            Hide
            rwijaya Rossiani Wijaya added a comment -

            This is working as expected.

            Test passed.

            Show
            rwijaya Rossiani Wijaya added a comment - This is working as expected. Test passed.
            Hide
            ppichet Pierre Pichet added a comment -

            See my last this last comment on MDL-29691
            This is related to MDL-33105 which have been applied to master before MDL-29691.

                public function apply_units($response, $separateunit = null) {
                    // Strip spaces (which may be thousands separators) and change other forms
                    // of writing e to e.
                    $response = str_replace(' ', '', $response);
                    // Strip thousand separators like half space.
                    if (!in_array($this->thousandssep, array(',', '.', ' '))) {
                        if (strpos($value, $this->thousandssep) !== false) {
                            $response = str_replace($this->thousandssep, '', $response);
                        }
                    }

            The code is not correct.
            $value should be respanced by $response.
            This imply to test for a thousandsep which could be not , . or regular space like halfspace.
            so I need to add additional test.
            I suggest you remove MDL-33105 so I can correct it and add supplementary tests

            Show
            ppichet Pierre Pichet added a comment - See my last this last comment on MDL-29691 This is related to MDL-33105 which have been applied to master before MDL-29691 . public function apply_units($response, $separateunit = null) { // Strip spaces (which may be thousands separators) and change other forms // of writing e to e. $response = str_replace(' ', '', $response); // Strip thousand separators like half space. if (!in_array($this->thousandssep, array(',', '.', ' '))) { if (strpos($value, $this->thousandssep) !== false) { $response = str_replace($this->thousandssep, '', $response); } } The code is not correct. $value should be respanced by $response. This imply to test for a thousandsep which could be not , . or regular space like halfspace. so I need to add additional test. I suggest you remove MDL-33105 so I can correct it and add supplementary tests
            Hide
            poltawski Dan Poltawski added a comment -

            Setting this to failed.

            So, am I understanding correctly that you think this should be reverted?

            Show
            poltawski Dan Poltawski added a comment - Setting this to failed. So, am I understanding correctly that you think this should be reverted?
            Hide
            poltawski Dan Poltawski added a comment -

            OK, reading again it seems clear, so reverting this change and reopening.

            Show
            poltawski Dan Poltawski added a comment - OK, reading again it seems clear, so reverting this change and reopening.
            Hide
            poltawski Dan Poltawski added a comment -

            I've reverted this change now. Thanks.

            Show
            poltawski Dan Poltawski added a comment - I've reverted this change now. Thanks.
            Hide
            cibot CiBoT added a comment -

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

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

            Thanks, you understand correctly and sorry for my typing errors...

            Show
            ppichet Pierre Pichet added a comment - Thanks, you understand correctly and sorry for my typing errors...
            Hide
            ppichet Pierre Pichet added a comment -

            I correct the code and add a new test for unusual thousandsep.
            Unit test Ok, code checker OK.
            As MDL-29691 is now on master, tests should include use of numerical in multianswer questiontype.
            More later on this

            Show
            ppichet Pierre Pichet added a comment - I correct the code and add a new test for unusual thousandsep. Unit test Ok, code checker OK. As MDL-29691 is now on master, tests should include use of numerical in multianswer questiontype. More later on this
            Hide
            timhunt Tim Hunt added a comment -

            Let me know when you have updated the testing instructions, and think this is ready for integration.

            Show
            timhunt Tim Hunt added a comment - Let me know when you have updated the testing instructions, and think this is ready for integration.
            Hide
            ppichet Pierre Pichet added a comment -

            On testing I discover that multianswer does not decode correctly number like 1 000,1 although it is OK for 1000,1
            The tests were done on MDL-29691 with 1000,1 as the numerical does not accept thousandseps.
            So I need to set the multianswer decoding ( I will create a new bug) before adding MDL-33105.

            Show
            ppichet Pierre Pichet added a comment - On testing I discover that multianswer does not decode correctly number like 1 000,1 although it is OK for 1000,1 The tests were done on MDL-29691 with 1000,1 as the numerical does not accept thousandseps. So I need to set the multianswer decoding ( I will create a new bug) before adding MDL-33105 .
            Hide
            ppichet Pierre Pichet added a comment -

            If I change the following lines in multianswer/questiontype.php

            @@ -260,11 +260,11 @@ define('ANSWER_ALTERNATIVE_REGEX_ANSWER', 3);
             define('ANSWER_ALTERNATIVE_REGEX_FEEDBACK', 5);
             
             // NUMBER_FORMATED_ALTERNATIVE_ANSWER_REGEX is used
             // for identifying numerical answers in ANSWER_ALTERNATIVE_REGEX_ANSWER
             define('NUMBER_REGEX',
            -        '-?(([0-9]+[.,]?[0-9]*|[.,][0-9]+)([eE][-+]?[0-9]+)?)');
            +        '-?(([0-9]+[., ]?[., 0-9]*|[.,][0-9]+)([eE][-+]?[0-9]+)?)');
             define('NUMERICAL_ALTERNATIVE_REGEX',
                     '^(' . NUMBER_REGEX . ')(:' . NUMBER_REGEX . ')?$');
             
             // Parenthesis positions for NUMERICAL_FORMATED_ALTERNATIVE_ANSWER_REGEX
             define('NUMERICAL_CORRECT_ANSWER', 1);

            then the decoding will correctly locate the answer and the tolerance in
            1 234.56 :0.1 or 1,234.56 or ....
            so the following numerical $ap->apply_units will give to the answer the correct value.

            Effectively the ,.or space can be mixed in any combinations.
            As we want flexibility and we do not grade number format, this is coherent.

            You could probably suggest a better regexp.

            Once the regexp set, I will build the supplementary tests.

            Show
            ppichet Pierre Pichet added a comment - If I change the following lines in multianswer/questiontype.php @@ -260,11 +260,11 @@ define('ANSWER_ALTERNATIVE_REGEX_ANSWER', 3); define('ANSWER_ALTERNATIVE_REGEX_FEEDBACK', 5); // NUMBER_FORMATED_ALTERNATIVE_ANSWER_REGEX is used // for identifying numerical answers in ANSWER_ALTERNATIVE_REGEX_ANSWER define('NUMBER_REGEX', - '-?(([0-9]+[.,]?[0-9]*|[.,][0-9]+)([eE][-+]?[0-9]+)?)'); + '-?(([0-9]+[., ]?[., 0-9]*|[.,][0-9]+)([eE][-+]?[0-9]+)?)'); define('NUMERICAL_ALTERNATIVE_REGEX', '^(' . NUMBER_REGEX . ')(:' . NUMBER_REGEX . ')?$'); // Parenthesis positions for NUMERICAL_FORMATED_ALTERNATIVE_ANSWER_REGEX define('NUMERICAL_CORRECT_ANSWER', 1); then the decoding will correctly locate the answer and the tolerance in 1 234.56 :0.1 or 1,234.56 or .... so the following numerical $ap->apply_units will give to the answer the correct value. Effectively the ,.or space can be mixed in any combinations. As we want flexibility and we do not grade number format, this is coherent. You could probably suggest a better regexp. Once the regexp set, I will build the supplementary tests.
            Hide
            timhunt Tim Hunt added a comment -

            That is regex is probably OK.

            Show
            timhunt Tim Hunt added a comment - That is regex is probably OK.
            Hide
            ppichet Pierre Pichet added a comment - - edited

            On further testing I realize that preceeding apply_unit was very strict about unit and number formatting.

            I need to test further how the new code handle bad responses or bad answer definition in creating multianswer numerical questions.

            I mostly tested that it was able to handle new numerical formats..

            Show
            ppichet Pierre Pichet added a comment - - edited On further testing I realize that preceeding apply_unit was very strict about unit and number formatting. I need to test further how the new code handle bad responses or bad answer definition in creating multianswer numerical questions. I mostly tested that it was able to handle new numerical formats..
            Hide
            ppichet Pierre Pichet added a comment -

            I just updated to actual master.
            I minimize relations to other question types.
            Some code checker errors are solved in bugs.

            Do you plan to put this on 2.5.1 ?
            Unit tests are OK.

            Show
            ppichet Pierre Pichet added a comment - I just updated to actual master. I minimize relations to other question types. Some code checker errors are solved in bugs. Do you plan to put this on 2.5.1 ? Unit tests are OK.
            Hide
            ppichet Pierre Pichet added a comment -

            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.

            To which version(s) should I rebase if you think that this is ready for testing and integrate.

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

            Show
            ppichet Pierre Pichet added a comment - 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. To which version(s) should I rebase if you think that this is ready for testing and integrate. If something goes wrong, I could possibly correct it in the next week.
            Hide
            timhunt Tim Hunt added a comment -

            Sorry, Pierre. I have badly behind on peer review. I will look later this afternoon.

            Show
            timhunt Tim Hunt added a comment - Sorry, Pierre. I have badly behind on peer review. I will look later this afternoon.
            Hide
            timhunt Tim Hunt added a comment -

            1. Fundamentally the new code looks good, so it is nearly ready for integration.

            but,

            2. You need to run codechecker on your changes.

            3. You don't need to change to list($value, $unit, $mult) = $this->ap->apply_units($x); It is OK if the list(...) bit of a line of code like this is shorter than the array returned.

            4. I think the testing instructions above would be easier to follow if you gave more explicit instructions for creating and testing one questions. You can then put at the end "try some other variants as used in the phpunit test, like -1 000 -1 000 000 -1.000.000 -1,000,000 -1000,100 -1.000,100 -1,000.100 3,14159"

            Show
            timhunt Tim Hunt added a comment - 1. Fundamentally the new code looks good, so it is nearly ready for integration. but, 2. You need to run codechecker on your changes. 3. You don't need to change to list($value, $unit, $mult) = $this->ap->apply_units($x); It is OK if the list(...) bit of a line of code like this is shorter than the array returned. 4. I think the testing instructions above would be easier to follow if you gave more explicit instructions for creating and testing one questions. You can then put at the end "try some other variants as used in the phpunit test, like -1 000 -1 000 000 -1.000.000 -1,000,000 -1000,100 -1.000,100 -1,000.100 3,14159"
            Hide
            timhunt Tim Hunt added a comment -

            Oops! I forgot to update the workflow state.

            Show
            timhunt Tim Hunt added a comment - Oops! I forgot to update the workflow state.
            Hide
            ppichet Pierre Pichet added a comment - - edited

            Tim,
            In edit_numerical the tolerance is not tested the same way as the answer

            if (!$this->is_valid_answer($trimmedanswer, $data)) {
                                $errors['answeroptions[' . $key . ']'] = $this->valid_answer_message($trimmedanswer);
                            }
                            if ($data['fraction'][$key] == 1) {
                                $maxgrade = true;
                            }
                            if ($answer !== '*' && !is_numeric($data['tolerance'][$key])) {
                                $errors['answeroptions['.$key.']'] =
                                        get_string('xmustbenumeric', 'qtype_numerical',
                                            get_string('acceptederror', 'qtype_numerical'));
                            }
                        } else if ($data['fraction'][$key] != 0 ||
                                !html_is_blank($data['feedback'][$key]['text'])) {
                            $errors['answeroptions[' . $key . ']'] = $this->valid_answer_message($trimmedanswer);
                            $answercount++;
                        }
            

            $repeatedoptions['tolerance']['type'] = PARAM_FLOAT;
            $repeatedoptions['answer']['type'] = PARAM_RAW;
            I modified the code so that tolerance value can be edited as the answer although without units...
            I will add some PHPunit tests and testing instructions.

            Show
            ppichet Pierre Pichet added a comment - - edited Tim, In edit_numerical the tolerance is not tested the same way as the answer if (!$this->is_valid_answer($trimmedanswer, $data)) { $errors['answeroptions[' . $key . ']'] = $this->valid_answer_message($trimmedanswer); } if ($data['fraction'][$key] == 1) { $maxgrade = true; } if ($answer !== '*' && !is_numeric($data['tolerance'][$key])) { $errors['answeroptions['.$key.']'] = get_string('xmustbenumeric', 'qtype_numerical', get_string('acceptederror', 'qtype_numerical')); } } else if ($data['fraction'][$key] != 0 || !html_is_blank($data['feedback'][$key]['text'])) { $errors['answeroptions[' . $key . ']'] = $this->valid_answer_message($trimmedanswer); $answercount++; } $repeatedoptions ['tolerance'] ['type'] = PARAM_FLOAT; $repeatedoptions ['answer'] ['type'] = PARAM_RAW; I modified the code so that tolerance value can be edited as the answer although without units... I will add some PHPunit tests and testing instructions.
            Hide
            ppichet Pierre Pichet added a comment -

            As form_test.php

            public function test_is_valid_number() {
                    $this->assertTrue($this->form->is_valid_number('1,001'));
                    $this->assertTrue($this->form->is_valid_number('1.001'));
                    $this->assertTrue($this->form->is_valid_number('1'));
                    $this->assertTrue($this->form->is_valid_number('1,e8'));
                    $this->assertFalse($this->form->is_valid_number('1001 xxx'));
                    $this->assertTrue($this->form->is_valid_number('1.e8'));
                }

            is testing is_valid_number() where the main test are done no supplementary tests appear necessary.

            Show
            ppichet Pierre Pichet added a comment - As form_test.php public function test_is_valid_number() { $this->assertTrue($this->form->is_valid_number('1,001')); $this->assertTrue($this->form->is_valid_number('1.001')); $this->assertTrue($this->form->is_valid_number('1')); $this->assertTrue($this->form->is_valid_number('1,e8')); $this->assertFalse($this->form->is_valid_number('1001 xxx')); $this->assertTrue($this->form->is_valid_number('1.e8')); } is testing is_valid_number() where the main test are done no supplementary tests appear necessary.
            Hide
            ppichet Pierre Pichet added a comment -

            Looking back I realize that I forgot to add the modifications in edit_multianswer_form for handling tolerances that I have done 3 months ago.
            https://github.com/ppichet/moodle/tree/MDL-33105c
            I will merge these in the final proposition.

            P.S. Working after midnight has some incoveniences...

            Show
            ppichet Pierre Pichet added a comment - Looking back I realize that I forgot to add the modifications in edit_multianswer_form for handling tolerances that I have done 3 months ago. https://github.com/ppichet/moodle/tree/MDL-33105c I will merge these in the final proposition. P.S. Working after midnight has some incoveniences...
            Hide
            ppichet Pierre Pichet added a comment -

            Finally I add the same flexibity when writing tolerance values.
            Only master have been updated.
            I will complete the testing instructions tomorrow.

            Show
            ppichet Pierre Pichet added a comment - Finally I add the same flexibity when writing tolerance values. Only master have been updated. I will complete the testing instructions tomorrow.
            Hide
            ppichet Pierre Pichet added a comment -

            sample questions for testing

            Show
            ppichet Pierre Pichet added a comment - sample questions for testing
            Hide
            ppichet Pierre Pichet added a comment -

            new examples to use when testing.
            There are 2 versions of the numerical question

            a first one with a unit (cm).
            the "fill the correct response" 1000.1 cm will not change when setting another language as french
            a second one without unit (sans unité)
            the "fill the correct response" will be
            will be 1000.1 when setting to english
            and
            1000,1 when setting another language as french

            The problem when there are a unit will be set (if not already done) when "finishing" MDL-31680

            Show
            ppichet Pierre Pichet added a comment - new examples to use when testing. There are 2 versions of the numerical question a first one with a unit (cm). the "fill the correct response" 1000.1 cm will not change when setting another language as french a second one without unit (sans unité) the "fill the correct response" will be will be 1000.1 when setting to english and 1000,1 when setting another language as french The problem when there are a unit will be set (if not already done) when "finishing" MDL-31680
            Hide
            ppichet Pierre Pichet added a comment -

            moodle xml version

            Show
            ppichet Pierre Pichet added a comment - moodle xml version
            Hide
            ppichet Pierre Pichet added a comment -

            Everything is OK on master.

            Show
            ppichet Pierre Pichet added a comment - Everything is OK on master.
            Hide
            timhunt Tim Hunt added a comment -

            Great! We got there in the end. Thanks for your persistence Pierre. I am now submitting this for integration.

            Show
            timhunt Tim Hunt added a comment - Great! We got there in the end. Thanks for your persistence Pierre. I am now submitting this for integration.
            Hide
            poltawski Dan Poltawski added a comment -

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

            TIA and ciao

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

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

            Show
            poltawski Dan Poltawski added a comment - I'm stopping this as i'd prefer to have Tim about when integrating this change.
            Hide
            poltawski Dan Poltawski added a comment -

            Integrated to master, thanks Pierre

            Show
            poltawski Dan Poltawski added a comment - Integrated to master, thanks Pierre
            Hide
            fred Frédéric Massart added a comment -

            Hi guys, sorry but I am failing this test for now.

            I found out that the thousands separator is not properly used in some cases. I was using the standard English language pack, but using the values:

            • 10,000
            • 200,000

            The comma is considered as a decimal separator, which should not be the case in en_US... though it is the right behaviour for fr_FR for example...

            Also, I noticed that the unit penalty and unit multiplier do not support the same number format as the answers. Isn't that a bit confusing? If I can use a comma as a decsep, which not there?

            I have to admit that I am not entirely convinced by the flexibility here. Of course I understand the purpose and I think that would be great if we could add any number in any format, but is that possible without randomising the behaviours?

            Not to mention that throughout Moodle we do not have this level of flexibility, and I think this could really confuse a teacher/student that can use a format in one place, but not in the other. From a user point of view there is no differentiation between a quiz or any other part of Moodle. How is the gradebook behaving by the way?

            I also found the code a bit confusing as a developer as, again, the flexibility is high and so the unit tests cases, though passing decsep and thousandsep to the answer processor, were not failing using other formats.

            Here is a patch adding some more tests to the unit tests:

            diff --git a/question/type/numerical/tests/answerprocessor_test.php b/question/type/numerical/tests/answerprocessor_test.php
            index 000df7a..cf88d46 100644
            --- a/question/type/numerical/tests/answerprocessor_test.php
            +++ b/question/type/numerical/tests/answerprocessor_test.php
            @@ -144,6 +144,8 @@ class qtype_numerical_answer_processor_test extends advanced_testcase {
                     $ap = new qtype_numerical_answer_processor(array(), false, ',', '.');
             
                     $this->assertEquals(array(-1000, '', null), $ap->apply_units('-1 000'));
            +        $this->assertEquals(array(-10000, '', null), $ap->apply_units('-10.000'));
            +        $this->assertEquals(array(-100000, '', null), $ap->apply_units('-100.000'));
                     $this->assertEquals(array(-1000000, '', null), $ap->apply_units('-1 000 000'));
                     $this->assertEquals(array(-1000000, '', null), $ap->apply_units('-1.000.000'));
                     $this->assertEquals(array(-1000000, '', null), $ap->apply_units('-1.000.000,'));
            @@ -153,6 +155,20 @@ class qtype_numerical_answer_processor_test extends advanced_testcase {
                     $this->assertEquals(array(3.14159, '', null), $ap->apply_units('3.14159'));
                     $this->assertEquals(array(3.14159, '', null), $ap->apply_units('3,14159'));
                 }
            +
            +    public function test_us_style() {
            +        $ap = new qtype_numerical_answer_processor(array(), false, '.', ',');
            +
            +        $this->assertEquals(array(-1000, '', null), $ap->apply_units('-1000'));
            +        $this->assertEquals(array(-10000, '', null), $ap->apply_units('-10,000'));
            +        $this->assertEquals(array(-100000, '', null), $ap->apply_units('-100,000'));
            +        $this->assertEquals(array(-1000000, '', null), $ap->apply_units('-1,000,000'));
            +        $this->assertEquals(array(-1000000, '', null), $ap->apply_units('-1,000,000.'));
            +        $this->assertEquals(array(-1000.1, '', null), $ap->apply_units('-1000.100'));
            +        $this->assertEquals(array(3.14159, '', null), $ap->apply_units('3.14159'));
            +        $this->assertEquals(array(30141.59, '', null), $ap->apply_units('30,141.59'));
            +    }
            +
                 public function test_unusual_thousandseps_style() {
                     // Test with ' as thousandsep as used for currency in Swizterland.
                     $ap = new qtype_numerical_answer_processor(array(), false, '.', "'");
            

            Thanks all!
            Fred

            Show
            fred Frédéric Massart added a comment - Hi guys, sorry but I am failing this test for now. I found out that the thousands separator is not properly used in some cases. I was using the standard English language pack, but using the values: 10,000 200,000 The comma is considered as a decimal separator, which should not be the case in en_US... though it is the right behaviour for fr_FR for example... Also, I noticed that the unit penalty and unit multiplier do not support the same number format as the answers. Isn't that a bit confusing? If I can use a comma as a decsep, which not there? I have to admit that I am not entirely convinced by the flexibility here. Of course I understand the purpose and I think that would be great if we could add any number in any format, but is that possible without randomising the behaviours? Not to mention that throughout Moodle we do not have this level of flexibility, and I think this could really confuse a teacher/student that can use a format in one place, but not in the other. From a user point of view there is no differentiation between a quiz or any other part of Moodle. How is the gradebook behaving by the way? I also found the code a bit confusing as a developer as, again, the flexibility is high and so the unit tests cases, though passing decsep and thousandsep to the answer processor, were not failing using other formats. Here is a patch adding some more tests to the unit tests: diff --git a/question/type/numerical/tests/answerprocessor_test.php b/question/type/numerical/tests/answerprocessor_test.php index 000df7a..cf88d46 100644 --- a/question/type/numerical/tests/answerprocessor_test.php +++ b/question/type/numerical/tests/answerprocessor_test.php @@ -144,6 +144,8 @@ class qtype_numerical_answer_processor_test extends advanced_testcase { $ap = new qtype_numerical_answer_processor(array(), false, ',', '.'); $this->assertEquals(array(-1000, '', null), $ap->apply_units('-1 000')); + $this->assertEquals(array(-10000, '', null), $ap->apply_units('-10.000')); + $this->assertEquals(array(-100000, '', null), $ap->apply_units('-100.000')); $this->assertEquals(array(-1000000, '', null), $ap->apply_units('-1 000 000')); $this->assertEquals(array(-1000000, '', null), $ap->apply_units('-1.000.000')); $this->assertEquals(array(-1000000, '', null), $ap->apply_units('-1.000.000,')); @@ -153,6 +155,20 @@ class qtype_numerical_answer_processor_test extends advanced_testcase { $this->assertEquals(array(3.14159, '', null), $ap->apply_units('3.14159')); $this->assertEquals(array(3.14159, '', null), $ap->apply_units('3,14159')); } + + public function test_us_style() { + $ap = new qtype_numerical_answer_processor(array(), false, '.', ','); + + $this->assertEquals(array(-1000, '', null), $ap->apply_units('-1000')); + $this->assertEquals(array(-10000, '', null), $ap->apply_units('-10,000')); + $this->assertEquals(array(-100000, '', null), $ap->apply_units('-100,000')); + $this->assertEquals(array(-1000000, '', null), $ap->apply_units('-1,000,000')); + $this->assertEquals(array(-1000000, '', null), $ap->apply_units('-1,000,000.')); + $this->assertEquals(array(-1000.1, '', null), $ap->apply_units('-1000.100')); + $this->assertEquals(array(3.14159, '', null), $ap->apply_units('3.14159')); + $this->assertEquals(array(30141.59, '', null), $ap->apply_units('30,141.59')); + } + public function test_unusual_thousandseps_style() { // Test with ' as thousandsep as used for currency in Swizterland. $ap = new qtype_numerical_answer_processor(array(), false, '.', "'"); Thanks all! Fred
            Hide
            timhunt Tim Hunt added a comment -

            Fred, I agree "I am not entirely convinced by the flexibility here". However, how long have you been thinking about this issue? Some of us have been worrying about it for years.

            You may think that language packs provide a solution, but you are wrong. Pierre is a teacher who works in Montreal, where they speak/write a mixture of English and French (probably most people speak both), so there will be a mixture of people in any given class, and then can understand the questions, whether they are written in French or English, but will write numbers the way they write them.

            So, if you think you can convince us of a solution that is better than the one proposed here, then please tell us exactly what it is, with your evidence that it will work better. If you are just guessing, then welcome to the world we have been in for the last N years.

            Finally, unit penalty and unit multiplier should be dealt with in a different issue.

            Show
            timhunt Tim Hunt added a comment - Fred, I agree "I am not entirely convinced by the flexibility here". However, how long have you been thinking about this issue? Some of us have been worrying about it for years. You may think that language packs provide a solution, but you are wrong. Pierre is a teacher who works in Montreal, where they speak/write a mixture of English and French (probably most people speak both), so there will be a mixture of people in any given class, and then can understand the questions, whether they are written in French or English, but will write numbers the way they write them. So, if you think you can convince us of a solution that is better than the one proposed here, then please tell us exactly what it is, with your evidence that it will work better. If you are just guessing, then welcome to the world we have been in for the last N years. Finally, unit penalty and unit multiplier should be dealt with in a different issue.
            Hide
            poltawski Dan Poltawski added a comment -

            I think Fred raises a good point about the inconsistency about this flexibility as a general issue, something i'd not properly considered whilst integrating this issue.

            I'm interested why we think the numerical type is a special case which should be handled differently to the rest of Moodle? Is this inconsistency going to cause more problems, should it instead be handled by the core functionality.

            I don't know and I certainly haven't been considering it at all, but I would be interested in having the rationale outlined here. Is it just because its variability of student answers?

            Show
            poltawski Dan Poltawski added a comment - I think Fred raises a good point about the inconsistency about this flexibility as a general issue, something i'd not properly considered whilst integrating this issue. I'm interested why we think the numerical type is a special case which should be handled differently to the rest of Moodle? Is this inconsistency going to cause more problems, should it instead be handled by the core functionality. I don't know and I certainly haven't been considering it at all, but I would be interested in having the rationale outlined here. Is it just because its variability of student answers?
            Hide
            timhunt Tim Hunt added a comment -

            Dan, Fred, how much stuff do you want to read? As I say, we have been thinking/talking about it for years. There are several tracker issues, forum threads and previous commits, but I am guessing you don't want to read all that.

            I guess you also want a nice simple logical rationale, that I can give you to explain why this solution is right. Well, sorry, there is no solution that is always right. The best we can do is a messy compromise that pleases most of the people most of the time. Maximising the meaning of most. That is what we have here.

            Yes, processing student answers is a special case. We can expect Teachers to spend at least a little time learning the right way to create things in Moodle. Also, teachers get the change to preview their quiz and ensure it is working, and they have a chance to fix it if not. So, for teachers, the standard (though not consistently implemented) PARAM_RAW + unformat_float to parse user input is OK.

            For students, doing a quiz, the situation is different:

            • We want the students to be concentrating on answering the question asked, to be thinking about their learning, not to be thinking about the technicalities of using Moodle.
            • Students are going to be graded on what they typed, with no chance to fix problems. It is much more important that we correctly interpret their meaning (even though it some cases there is genuine ambiguity) rather than that we follow some simple, logical Moodle coding pattern (PARAM_RAW + unformat_float) that works less well.
            Show
            timhunt Tim Hunt added a comment - Dan, Fred, how much stuff do you want to read? As I say, we have been thinking/talking about it for years. There are several tracker issues, forum threads and previous commits, but I am guessing you don't want to read all that. I guess you also want a nice simple logical rationale, that I can give you to explain why this solution is right. Well, sorry, there is no solution that is always right. The best we can do is a messy compromise that pleases most of the people most of the time. Maximising the meaning of most. That is what we have here. Yes, processing student answers is a special case. We can expect Teachers to spend at least a little time learning the right way to create things in Moodle. Also, teachers get the change to preview their quiz and ensure it is working, and they have a chance to fix it if not. So, for teachers, the standard (though not consistently implemented) PARAM_RAW + unformat_float to parse user input is OK. For students, doing a quiz, the situation is different: We want the students to be concentrating on answering the question asked, to be thinking about their learning, not to be thinking about the technicalities of using Moodle. Students are going to be graded on what they typed, with no chance to fix problems. It is much more important that we correctly interpret their meaning (even though it some cases there is genuine ambiguity) rather than that we follow some simple, logical Moodle coding pattern (PARAM_RAW + unformat_float) that works less well.
            Hide
            fred Frédéric Massart added a comment -

            Tim, I am certainly new to this issue, and I am just expressing my thoughts. The reason why I failed this test is because it didn't behave as expected, not because "I am not convinced". The English vs. French is a very good point, and that's exactly where the problem was during my testing. I speak French and I've used commas as a decimal separator for years, but to come back to your example, what is the expected behaviour, in Montral, in a bilingual environment, in a flexible Moodle, when you enter:

            10,001

            • If I speak English, I probably expect: int 10001
            • If I speak French, I expect: float 10.001

            And I don't mention the surprise of a student who ends up failing his quiz because he mistakenly assumed that Moodle knew what he was doing.

            Perhaps the solution does not only reside in the language pack, but when you install Windows/Mac/Linux using a specific locale, you don't expect Excel and friends to guess what format you decided to use when filling in your document, you're expected to use the chosen locale.

            I understand the problematic, and I understand the attempt of solution, and surely it'll help users in Montreal, but it could also create loads of confusion in other environments.

            Perhaps we should work on a core solution rather than a specific numerical question answers, and I personally don't think the unit penalty and multiplier should be dealt with in a different issue. And if they should, then they should be tied together because having 2 different behaviours, especially on the same page, would definitely confuse users more than it will help them.

            What about the HTML5 input element for floats? What rules does it follow?

            Show
            fred Frédéric Massart added a comment - Tim, I am certainly new to this issue, and I am just expressing my thoughts. The reason why I failed this test is because it didn't behave as expected, not because "I am not convinced". The English vs. French is a very good point, and that's exactly where the problem was during my testing. I speak French and I've used commas as a decimal separator for years, but to come back to your example, what is the expected behaviour, in Montral, in a bilingual environment, in a flexible Moodle, when you enter: 10,001 If I speak English, I probably expect: int 10001 If I speak French, I expect: float 10.001 And I don't mention the surprise of a student who ends up failing his quiz because he mistakenly assumed that Moodle knew what he was doing. Perhaps the solution does not only reside in the language pack, but when you install Windows/Mac/Linux using a specific locale, you don't expect Excel and friends to guess what format you decided to use when filling in your document, you're expected to use the chosen locale. I understand the problematic, and I understand the attempt of solution, and surely it'll help users in Montreal, but it could also create loads of confusion in other environments. Perhaps we should work on a core solution rather than a specific numerical question answers , and I personally don't think the unit penalty and multiplier should be dealt with in a different issue. And if they should, then they should be tied together because having 2 different behaviours, especially on the same page, would definitely confuse users more than it will help them. What about the HTML5 input element for floats? What rules does it follow?
            Hide
            timhunt Tim Hunt added a comment -

            I looked at the spec for HTML number input, and it almost completely ignores this issue

            If you look at the data (e.g. review all answers given by all students to all numerical questions in the database of a large Moodle site) then you will find that students hardly ever type thousand separators. Therefore, it is much more important to get decimal handling right.

            Show
            timhunt Tim Hunt added a comment - I looked at the spec for HTML number input, and it almost completely ignores this issue If you look at the data (e.g. review all answers given by all students to all numerical questions in the database of a large Moodle site) then you will find that students hardly ever type thousand separators. Therefore, it is much more important to get decimal handling right.
            Hide
            fred Frédéric Massart added a comment -

            Maybe we should ignore the thousands separator entirely then, and focus on the decimal one, at least for the user input. What scares me is the automatic assumption, I think it's safer, even if annoying for a student, to throw out a validation error once we have some uncertainty (multiple commas, weird pattern, etc...).

            Show
            fred Frédéric Massart added a comment - Maybe we should ignore the thousands separator entirely then, and focus on the decimal one, at least for the user input. What scares me is the automatic assumption, I think it's safer, even if annoying for a student, to throw out a validation error once we have some uncertainty (multiple commas, weird pattern, etc...).
            Hide
            damyon Damyon Wiese added a comment -

            What about using the HTTP Accept-Language header to get the decsep value from any installed language pack with a fall back to the current language pack setting?

            Show
            damyon Damyon Wiese added a comment - What about using the HTTP Accept-Language header to get the decsep value from any installed language pack with a fall back to the current language pack setting?
            Hide
            fred Frédéric Massart added a comment -

            What if you have your Moodle in French/Chinese/English but you are forced to use your Uni's computers set to whatever locale? Thus sending a mistakenly wrong Accept-Language header. The behaviour is even less predictable for a user jumping from multiple Moodle instances, which won't all have the same locale installed.

            Show
            fred Frédéric Massart added a comment - What if you have your Moodle in French/Chinese/English but you are forced to use your Uni's computers set to whatever locale? Thus sending a mistakenly wrong Accept-Language header. The behaviour is even less predictable for a user jumping from multiple Moodle instances, which won't all have the same locale installed.
            Hide
            timhunt Tim Hunt added a comment -

            Moodle does use Accept-Language to set the user's current language, if you have turned on that admin option. I don't think any other part of Moodle should touch Accept-Language.

            Fred, "Maybe we should ignore the thousands separator entirely then" what exactly do you mean?

            Show
            timhunt Tim Hunt added a comment - Moodle does use Accept-Language to set the user's current language, if you have turned on that admin option. I don't think any other part of Moodle should touch Accept-Language. Fred, "Maybe we should ignore the thousands separator entirely then" what exactly do you mean?
            Hide
            fred Frédéric Massart added a comment -

            I mean that we should not try to guess what are the thousands separators. We could simply say that if there is

            • 1 comma == decimal separator
            • 1 dot == decimal separator
            • >= 1 dot or >= 1 comma == error

            Of course there could still be some confusion when entering figures using the thousands separator, but I would assume that currently all over Moodle we are not allowing users to enter any thousands separator anywhere. We could set a rule where thousands separators are never accepted, and enforce our users to learn not to use them ever. In fact, rather than being very flexible, we could be helpful... either by throwing validation warnings, or Javascript smart validation (Did you mean 10.000?).

            (Not sure I made myself clear :/)

            Show
            fred Frédéric Massart added a comment - I mean that we should not try to guess what are the thousands separators. We could simply say that if there is 1 comma == decimal separator 1 dot == decimal separator >= 1 dot or >= 1 comma == error Of course there could still be some confusion when entering figures using the thousands separator, but I would assume that currently all over Moodle we are not allowing users to enter any thousands separator anywhere. We could set a rule where thousands separators are never accepted, and enforce our users to learn not to use them ever. In fact, rather than being very flexible, we could be helpful ... either by throwing validation warnings, or Javascript smart validation (Did you mean 10.000?). (Not sure I made myself clear :/)
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            I am sorry but I am completely lost by comments from Frederic, Dan or Daymon !
            Shouldn't this issue concentrate on the situation before and after the patch and also on the title of this issue Numerical is_complete() could return a false on a good response
            It seems to me the discussion is changing on a completely different subject about completely changing the rules for answers in numerical question type

            • If this discussion is to take place surely a new tracker issue should be better
            • If this discussion is to take place surely it should not be when the most knowledgeable person on the subject (Pierre) is on holidays
            • If this discussion is to take place surely the final decision must take care of not break thousands of questions and attempts around the world
            • If this discussion is to take place surely some teachers should be part of it not only developers

            Fortunately it happen I am a teacher
            To summarize most of my students are French, but some are English, German, Spanish and Chinese. Most of my course use French language but each student can choose their own language, but some are in English with no possibility to change the language.
            Imagine you are a Chinese student in one of my course with French content but Moodle interface switched to Chinese, how do you expect answers like 20.45 or 20,45 to be graded ?
            Imagine another Chinese student in the same course but with Moodle interface in French (because he is a good student and want to preactice French as much as he can), would you expect him to be graded differently for the same answers than the previous student ?

            I can continue forever.

            My +1,000 (or +1.000 or + 1 000 or + 1000,00 or +1000.00, or ...) no to make any too quick change without fully understanding the consequences.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - I am sorry but I am completely lost by comments from Frederic, Dan or Daymon ! Shouldn't this issue concentrate on the situation before and after the patch and also on the title of this issue Numerical is_complete() could return a false on a good response It seems to me the discussion is changing on a completely different subject about completely changing the rules for answers in numerical question type If this discussion is to take place surely a new tracker issue should be better If this discussion is to take place surely it should not be when the most knowledgeable person on the subject (Pierre) is on holidays If this discussion is to take place surely the final decision must take care of not break thousands of questions and attempts around the world If this discussion is to take place surely some teachers should be part of it not only developers Fortunately it happen I am a teacher To summarize most of my students are French, but some are English, German, Spanish and Chinese. Most of my course use French language but each student can choose their own language, but some are in English with no possibility to change the language. Imagine you are a Chinese student in one of my course with French content but Moodle interface switched to Chinese, how do you expect answers like 20.45 or 20,45 to be graded ? Imagine another Chinese student in the same course but with Moodle interface in French (because he is a good student and want to preactice French as much as he can), would you expect him to be graded differently for the same answers than the previous student ? I can continue forever. My +1,000 (or +1.000 or + 1 000 or + 1000,00 or +1000.00, or ...) no to make any too quick change without fully understanding the consequences.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            I am completely with Fred on "we could be helpful... either by throwing validation warnings, or Javascript smart validation".
            In the formulas question type where answers can not only be numerical but also algebraic formulas, as soon as the content is illegal (not wrong or correct but illegal according to the parser) a small icon (a red triangular warning sign) appears. This is done using javascript and also as long as the answer's content is correctly parsed the script display how it understand the student answer. See the first screenshot on this page http://code.google.com/p/moodle-coordinate-question/

            Show
            jmvedrine Jean-Michel Vedrine added a comment - I am completely with Fred on "we could be helpful... either by throwing validation warnings, or Javascript smart validation". In the formulas question type where answers can not only be numerical but also algebraic formulas, as soon as the content is illegal (not wrong or correct but illegal according to the parser) a small icon (a red triangular warning sign) appears. This is done using javascript and also as long as the answer's content is correctly parsed the script display how it understand the student answer. See the first screenshot on this page http://code.google.com/p/moodle-coordinate-question/
            Hide
            poltawski Dan Poltawski added a comment -

            Is it feasible to do freds idea and enforce only a decimal separator and not allow a thousands separator?

            A few of us have talked about it here and quite like that idea?

            Show
            poltawski Dan Poltawski added a comment - Is it feasible to do freds idea and enforce only a decimal separator and not allow a thousands separator? A few of us have talked about it here and quite like that idea?
            Hide
            timhunt Tim Hunt added a comment -

            Given the complexity of this issue I feel that "enforce only a decimal separator and not allow a thousands separator" is rather a vague statement. I will happily review/comment on any patch that Fred cares to write, even if it is only a proof concept.

            Show
            timhunt Tim Hunt added a comment - Given the complexity of this issue I feel that "enforce only a decimal separator and not allow a thousands separator" is rather a vague statement. I will happily review/comment on any patch that Fred cares to write, even if it is only a proof concept.
            Hide
            timhunt Tim Hunt added a comment - - edited

            Actually, Fred should realise his idea will not work. The key example is 10,001 (or 10.001)

            As Fred says

            If I speak English, I probably expect: int 10001
            If I speak French, I expect: float 10.001

            (or vice versa).

            If we could tell what was a decimal separator, or a thousands separator, in the student's response, then this issue would not exist. The whole point is that we can't. Anyway, Fred's suggestion does not work. I wish it did.

            Show
            timhunt Tim Hunt added a comment - - edited Actually, Fred should realise his idea will not work. The key example is 10,001 (or 10.001) As Fred says If I speak English, I probably expect: int 10001 If I speak French, I expect: float 10.001 (or vice versa). If we could tell what was a decimal separator, or a thousands separator, in the student's response, then this issue would not exist. The whole point is that we can't. Anyway, Fred's suggestion does not work. I wish it did.
            Hide
            poltawski Dan Poltawski added a comment -

            Yeah, I was just coming to that conclusion myself. I see some of the issues is already covered in here MDL-31889.

            I'm not totally convinced here, but I defer to Tim's jugement as quiz maintainer.

            I think Fred's point about consistency is important. When students start marking each others work in workshop, can they continue to use this same flexibility in grade values (or in any other modules where students handle floating point numbers)? Is partial, inconsistent flexibility (over the whole of Moodle) an improvement?

            Show
            poltawski Dan Poltawski added a comment - Yeah, I was just coming to that conclusion myself. I see some of the issues is already covered in here MDL-31889 . I'm not totally convinced here, but I defer to Tim's jugement as quiz maintainer. I think Fred's point about consistency is important. When students start marking each others work in workshop, can they continue to use this same flexibility in grade values (or in any other modules where students handle floating point numbers)? Is partial, inconsistent flexibility (over the whole of Moodle) an improvement?
            Hide
            fred Frédéric Massart added a comment - - edited

            Don't mention my name as if I was on a Crusade, I am just trying to express some concerns about the usability of such a flexibility, I am not suggesting that "my" suggestions are appropriate. Neither that I can/want to provide a patch satisfying everyone...

            On a side note, what I meant by ignoring the thousand separator, was:

            • 1 comma == decimal separator
            • 1 dot == decimal separator
            • >= 1 dot or >= 1 comma == error

            And educating the users in the sense that any separator used would be considered as decimal one, except if when too ambiguous (multiple separators, too many of the same, ...). And not (as it is at the moment), a comma could be both (10,000.00 vs. 10,000).

            Anyway, case closed.

            Show
            fred Frédéric Massart added a comment - - edited Don't mention my name as if I was on a Crusade, I am just trying to express some concerns about the usability of such a flexibility, I am not suggesting that "my" suggestions are appropriate. Neither that I can/want to provide a patch satisfying everyone... On a side note, what I meant by ignoring the thousand separator, was: 1 comma == decimal separator 1 dot == decimal separator >= 1 dot or >= 1 comma == error And educating the users in the sense that any separator used would be considered as decimal one, except if when too ambiguous (multiple separators, too many of the same, ...). And not (as it is at the moment), a comma could be both (10,000.00 vs. 10,000). Anyway, case closed.
            Hide
            fred Frédéric Massart added a comment -

            I think we could at least add some failing unit test (commented out) to this issue, to document the problem.

            Show
            fred Frédéric Massart added a comment - I think we could at least add some failing unit test (commented out) to this issue, to document the problem.
            Hide
            timhunt Tim Hunt added a comment -

            I'm sorry Fred. I really should not react like that. In fact, just after I was rude to you, in the next tracker issue Petr made an unfriendly response to one of my peer review comments, and I did not like that, so I really should know better. We are all trying our best to make Moodle better.

            So anyway, the case your suggestion is about is 1,000,000.0, or 1.000.000,0. The point about that case is that it is not ambiguous.

            Also, I think I am getting confused between this issue, and MDL-31889, which is where this issue is addressed properly.

            Show
            timhunt Tim Hunt added a comment - I'm sorry Fred. I really should not react like that. In fact, just after I was rude to you, in the next tracker issue Petr made an unfriendly response to one of my peer review comments, and I did not like that, so I really should know better. We are all trying our best to make Moodle better. So anyway, the case your suggestion is about is 1,000,000.0, or 1.000.000,0. The point about that case is that it is not ambiguous. Also, I think I am getting confused between this issue, and MDL-31889 , which is where this issue is addressed properly.
            Hide
            poltawski Dan Poltawski added a comment -

            Just attaching this image to demonstrate one of the problems

            Show
            poltawski Dan Poltawski added a comment - Just attaching this image to demonstrate one of the problems
            Hide
            poltawski Dan Poltawski added a comment -

            And this is what happened before this week, feels like a regression to me:

            Show
            poltawski Dan Poltawski added a comment - And this is what happened before this week, feels like a regression to me:
            Hide
            fred Frédéric Massart added a comment -

            Attaching a Quiz demonstrating the issue I didn't appear to make clear in my previous comments.

            Show
            fred Frédéric Massart added a comment - Attaching a Quiz demonstrating the issue I didn't appear to make clear in my previous comments.
            Hide
            timhunt Tim Hunt added a comment -

            Yes, that is the one case that the new code handles less well than the old code. There are other examples where the reverse is true.

            Do you want to revert this until Pierre gets back from holiday? This is 2.6-only, so we have time.

            Show
            timhunt Tim Hunt added a comment - Yes, that is the one case that the new code handles less well than the old code. There are other examples where the reverse is true. Do you want to revert this until Pierre gets back from holiday? This is 2.6-only, so we have time.
            Hide
            poltawski Dan Poltawski added a comment -

            OK, i've reverted this now.

            I have to say, the more I think about it, the more I think its an unsolvable problem to interpret their meaning with both notations at once. Additionally it makes me wonder if that is really teaching bad practice (i'm sure such tolerance is allowed on a tax form).

            Would we not be better making it clear what the expected separator is, or something like that to avoid ambiguity?

            I'm asking more questions to this tricky issue..

            Show
            poltawski Dan Poltawski added a comment - OK, i've reverted this now. I have to say, the more I think about it, the more I think its an unsolvable problem to interpret their meaning with both notations at once. Additionally it makes me wonder if that is really teaching bad practice (i'm sure such tolerance is allowed on a tax form). Would we not be better making it clear what the expected separator is, or something like that to avoid ambiguity? I'm asking more questions to this tricky issue..
            Hide
            cibot CiBoT added a comment -

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

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

            Well, the one question type I know that gets this really right is STACK.

            Maths notation is even more ambiguous than just numbers (e.g. x(t+1), is that a function call or multiplication? what about 2x, or 2½?)

            The approach in STACK is when you types your response, there is some Ajax code saying "Your answer was interpreted as ..." with a nicely rendered equation. You can then either accept that, or cahnge your response if it was misinterpreted.

            Isn't that over-kill for a simple numerical question? Surely it is. But then will anything less do?

            Show
            timhunt Tim Hunt added a comment - Well, the one question type I know that gets this really right is STACK. Maths notation is even more ambiguous than just numbers (e.g. x(t+1), is that a function call or multiplication? what about 2x, or 2½?) The approach in STACK is when you types your response, there is some Ajax code saying "Your answer was interpreted as ..." with a nicely rendered equation. You can then either accept that, or cahnge your response if it was misinterpreted. Isn't that over-kill for a simple numerical question? Surely it is. But then will anything less do?
            Hide
            ppichet Pierre Pichet added a comment -

            Thanks everybody for their comments.
            A short comment from Paris...
            The rule is that thousandsep should not be used without the decsep although this rule can be relieved if there are more than a single thousandsep.

            This should be shown to the student (or teacher) in some way.

            With this rule we CAN show to the student the correct response in the language set.
            The correct response does not show the thousandsep that are only allowed for convenience on input.
            The numerical and calculated questions are not graded for the format used.

            More on return.
            P.S Living in a bilingual state, my students are allowed to set the moodle course to either french or english.

            Show
            ppichet Pierre Pichet added a comment - Thanks everybody for their comments. A short comment from Paris... The rule is that thousandsep should not be used without the decsep although this rule can be relieved if there are more than a single thousandsep. This should be shown to the student (or teacher) in some way. With this rule we CAN show to the student the correct response in the language set. The correct response does not show the thousandsep that are only allowed for convenience on input. The numerical and calculated questions are not graded for the format used. More on return. P.S Living in a bilingual state, my students are allowed to set the moodle course to either french or english.
            Hide
            ppichet Pierre Pichet added a comment - - edited

            Moodle 2.6 being solved this weekend, let's go back to this issue.
            We need to agree that the main use of the actual numerical questiontype is to grade the number value within a +- range and not the way the numerical value is written. This should be as much flexible as it could be without breaking the millions of already existing questions.
            So one thousand should continue to be expressed as 1000 1,000.00 1 000, 1.000,
            To remove the ambiguity we need to set a way that the user who want to use the thousand separator should ALWAYS put the decimal sep even if the code could detect the case when the numbrer is 1,000,000. or greater.
            Where do we put this rule ?

            1. in an help button alongside the question text as was done in Moodle 2.0. However this is not easy to use for numerical questions in Cloze questions
            2. as a specific comment that expressed in the good response text the numerical value of the student response as seen by moodle.
            3. or ?
            Show
            ppichet Pierre Pichet added a comment - - edited Moodle 2.6 being solved this weekend, let's go back to this issue. We need to agree that the main use of the actual numerical questiontype is to grade the number value within a +- range and not the way the numerical value is written. This should be as much flexible as it could be without breaking the millions of already existing questions. So one thousand should continue to be expressed as 1000 1,000.00 1 000, 1.000, To remove the ambiguity we need to set a way that the user who want to use the thousand separator should ALWAYS put the decimal sep even if the code could detect the case when the numbrer is 1,000,000. or greater. Where do we put this rule ? in an help button alongside the question text as was done in Moodle 2.0. However this is not easy to use for numerical questions in Cloze questions as a specific comment that expressed in the good response text the numerical value of the student response as seen by moodle. or ?
            Hide
            ppichet Pierre Pichet added a comment -

            If among your other (effectively more important) duties, you could suggest how to resolve the apparent conflict between

            • the historical flexibility of numerical question grading i.e. 1,000 as 1.000
              and
            • the initial reaction of english user to grade "correctly" a written answer of 1,000 as one thousand .
            Show
            ppichet Pierre Pichet added a comment - If among your other (effectively more important) duties, you could suggest how to resolve the apparent conflict between the historical flexibility of numerical question grading i.e. 1,000 as 1.000 and the initial reaction of english user to grade "correctly" a written answer of 1,000 as one thousand .
            Hide
            timhunt Tim Hunt added a comment -

            I think the key point is that, in cases where we can still interact with the student, we should not remove the old validation message (https://tracker.moodle.org/browse/MDL-33105?focusedCommentId=229715&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-229715) that encourages the student to input their answer with no thousands separator, and using the right decimal point character for the current language.

            It is only if they ingore that warning, and click Submit all and finish anyway, that we should then do our best to grade their response.

            Does that seem reasonable?

            Show
            timhunt Tim Hunt added a comment - I think the key point is that, in cases where we can still interact with the student, we should not remove the old validation message ( https://tracker.moodle.org/browse/MDL-33105?focusedCommentId=229715&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-229715 ) that encourages the student to input their answer with no thousands separator, and using the right decimal point character for the current language. It is only if they ingore that warning, and click Submit all and finish anyway, that we should then do our best to grade their response. Does that seem reasonable?
            Hide
            ppichet Pierre Pichet added a comment -

            Effectively not using is the simplest rule but people somehow want to use the thousand sep.

            I will try to figure all the most plausible combinations ( i.e. bilingual courses) and put the result in the developper docs to help us see a clearer picture.
            We could put more efforts in decoding the number as 1,23 or 1,2345 are clearly using , as a decimal point.

            Show
            ppichet Pierre Pichet added a comment - Effectively not using is the simplest rule but people somehow want to use the thousand sep. I will try to figure all the most plausible combinations ( i.e. bilingual courses) and put the result in the developper docs to help us see a clearer picture. We could put more efforts in decoding the number as 1,23 or 1,2345 are clearly using , as a decimal point.
            Hide
            raymor Ray Morris added a comment -

            It would be interesting to find out from others what percentage of their numerical questions have integer answers. In our case, most of numerical questions are integer. Therefore, 1,000 is very likely "one thousand". If we find that 95%+ of numerical questions don't deal with decimals, we could say that interpreting 1,000 as one thousand is probably correct 95% of the time or more.

            If anyone wants to check their databases, I'll save you ten seconds writing the queries:
            SELECT COUNT FROM mdl_question_answers qa, mdl_question_numerical
            WHERE mdl_question_numerical.answer=qa.id
            SELECT COUNT FROM mdl_question_answers qa, mdl_question_numerical
            WHERE mdl_question_numerical.answer=qa.id AND qa.answer LIKE '%.%'

            Show
            raymor Ray Morris added a comment - It would be interesting to find out from others what percentage of their numerical questions have integer answers. In our case, most of numerical questions are integer. Therefore, 1,000 is very likely "one thousand". If we find that 95%+ of numerical questions don't deal with decimals, we could say that interpreting 1,000 as one thousand is probably correct 95% of the time or more. If anyone wants to check their databases, I'll save you ten seconds writing the queries: SELECT COUNT FROM mdl_question_answers qa, mdl_question_numerical WHERE mdl_question_numerical.answer=qa.id SELECT COUNT FROM mdl_question_answers qa, mdl_question_numerical WHERE mdl_question_numerical.answer=qa.id AND qa.answer LIKE '%.%'
            Hide
            ppichet Pierre Pichet added a comment -

            The testing of valid number form could be more complete on regular numerical ( and calculated) questions if we add a grading for number format.
            This was not put formward because it would complexify the question editing interface.
            Since this proposal, the interface has become more easier to use and we could perhaps add this option about valid number format even if this could not be applied to numerical used inside Cloze question type.
            But this could settle this bug in a definitive way.

            Show
            ppichet Pierre Pichet added a comment - The testing of valid number form could be more complete on regular numerical ( and calculated) questions if we add a grading for number format. This was not put formward because it would complexify the question editing interface. Since this proposal, the interface has become more easier to use and we could perhaps add this option about valid number format even if this could not be applied to numerical used inside Cloze question type. But this could settle this bug in a definitive way.
            Hide
            marina Marina Glancy added a comment -

            I unassigned it from Pierre so somebody else can pick it since there was no activity on the issue for a while.

            Show
            marina Marina Glancy added a comment - I unassigned it from Pierre so somebody else can pick it since there was no activity on the issue for a while.
            Hide
            ppichet Pierre Pichet added a comment -

            There was no activity because there is no easy solution for this somehow minor inconvenience.
            I am just working on other aspects of calculated.
            I any case I never think that being the assignee as an exclusivity on working on a given problem probably because
            I am working on quite specialize problems like "calculated".

            Show
            ppichet Pierre Pichet added a comment - There was no activity because there is no easy solution for this somehow minor inconvenience. I am just working on other aspects of calculated. I any case I never think that being the assignee as an exclusivity on working on a given problem probably because I am working on quite specialize problems like "calculated".
            Hide
            marina Marina Glancy added a comment -

            Hi Pierre, being assignee of course does not mean working on the issue exclusively. However we often have situation in tracker when some issue was reopened (for example, through failing peer or integration review), and it remains with status "Development in process" and non-empty Assignee field. Other people don't touch it because they think it is being worked on and they don't realise that this status did not change for years.
            By all means I do not mind you continuing working on the issue. Feel free to assign it back to yourself.

            Show
            marina Marina Glancy added a comment - Hi Pierre, being assignee of course does not mean working on the issue exclusively. However we often have situation in tracker when some issue was reopened (for example, through failing peer or integration review), and it remains with status "Development in process" and non-empty Assignee field. Other people don't touch it because they think it is being worked on and they don't realise that this status did not change for years. By all means I do not mind you continuing working on the issue. Feel free to assign it back to yourself.
            Hide
            ppichet Pierre Pichet added a comment -

            Hi Marina, I effectively understood your monitoring process and that I was not excluded to work on this issue.

            Show
            ppichet Pierre Pichet added a comment - Hi Marina, I effectively understood your monitoring process and that I was not excluded to work on this issue.
            Hide
            ppichet Pierre Pichet added a comment -

            This handling of , is coming to the surface as related to MDL-49401.

            Show
            ppichet Pierre Pichet added a comment - This handling of , is coming to the surface as related to MDL-49401.

              People

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

                Dates

                • Created:
                  Updated: