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

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.

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.

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

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

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.

Stay in the mountains as long as you can

This can wait as long as you want.

There is no hurry.

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?

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

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.

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

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.

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

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.

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

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

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

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.

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

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

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.

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\

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.

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.

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.

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

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

Tim,
Its working ...

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

Si I passed the test

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

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

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

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.

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.

The phpunit tests seem OK.

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.

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 .

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.

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.

"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.

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

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.

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

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.

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.

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

"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 .

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 ?

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?

I updated the testing instruction field.

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

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 ?

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

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

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

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.

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

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

ping?

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.

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

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

Integrated to master only. Thanks Pierre.

This is working as expected.

Test passed.

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

Setting this to failed.

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

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

I've reverted this change now. Thanks.

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

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

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

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

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.

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.

That is regex is probably OK.

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

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.

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.

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

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"

Oops! I forgot to update the workflow state.

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.

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.

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

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

sample questions for testing

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

moodle xml version

Everything is OK on master.

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

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

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

Integrated to master, thanks Pierre

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

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.

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?

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.

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?

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.

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

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?

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.

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?

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 :/)

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.

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/

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?

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.

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.

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?

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.

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

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.

Just attaching this image to demonstrate one of the problems

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

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

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.

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

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

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?

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.

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 ?

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 .

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?

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.