Details

Type: Bug

Status: Closed

Priority: Minor

Resolution: Fixed

Affects Version/s: 2.0.2, 2.2.4, 2.3.1, 2.4

Component/s: Questions

Labels:

Testing Instructions:Hide
1.a Create a numerical question with tolerance 0, correct answer 0.
1.b Check that 0 is graded right, and 1e20 is graded wrong.
2.a Create a numerical question with tolerance 1e24, correct answer 0.
2.b Check that 0 and 1e24 are graded right and 1e23 is graded wrong.
3.a Create a numerical question with tolerance 0, correct answer 1e20.
3.b Check that 1e20 and 1.0000000001e20 are graded right, and 1.000001e20 is graded wrong.
4.a Create a numerical question with tolerance 1e24, correct answer 1e20.
4.b Check that 1e20 and 1.0001e20 are graded right, and 1.0002e20 is graded wrong.
Show1.a Create a numerical question with tolerance 0, correct answer 0. 1.b Check that 0 is graded right, and 1e20 is graded wrong. 2.a Create a numerical question with tolerance 1e24, correct answer 0. 2.b Check that 0 and 1e24 are graded right and 1e23 is graded wrong. 3.a Create a numerical question with tolerance 0, correct answer 1e20. 3.b Check that 1e20 and 1.0000000001e20 are graded right, and 1.000001e20 is graded wrong. 4.a Create a numerical question with tolerance 1e24, correct answer 1e20. 4.b Check that 1e20 and 1.0001e20 are graded right, and 1.0002e20 is graded wrong. 
Affected Branches:MOODLE_20_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE

Fixed Branches:MOODLE_22_STABLE, MOODLE_23_STABLE

Pull from Repository:

Pull Master Branch:

Pull Master Diff URL:

Rank:38478
Description
When the answer tolerance is set to absolute type the calculation to take in account the PHP precision is not correctly done for small values.
ex answer = 8.33e24 tolerance 0 actual range value calculated 9.999999991664E15 to 1.0000000008336E14
There was a small mismatch when transfering the calculations from the numerical/questiontype.php to new engine numerical/question.php
Activity
Hi Joshua,
I will come back to work mid august. So this could be fixed, if necessary, for your and my fall session as 8,33e24 seems to be at the atomic scale...
Before this, I will look at the problem to locate where the code has changed.
Good call on the guess. It was actually the energy of a photon.
Thanks for responding on your vacation. I appreciate you working on this.
Effectively the 1,9 does not account for th PHP precision limit when the tolerance is absolute not relative.
Before moving back 2 coding to 1,9 , there is a need to an extended analysis of the various possibilities related to absolute and relative errors.
The general moodle philosophy is to build the most flexible interface so that it satifies all teacher needs.
However real life has shown that teachers could need some guidance
As an example: Why do you use absolute 0 absolute tolerance i.e the student needs to give you the EXACT value with no more decimals than you set?
He could use a hand calculator that is more precise than the Moodle PHP.
My students are using a hand calculator. This question (and several other similar ones) are part of quizzes designed to serve as an interactive homework check system.
The general pattern for my class is that I give full credit for the correct answer with the correct significant figures and reduced credit for the correct answer with the wrong significant figures.
However, that 0 tolerance isn't the real problem here. Even if the tolerance is not 0 the error still appears. If I change it to 8.33E24 with tolerance of 2E24 and put in the answer as 4E24 which is outside the tolerance range, it will still mark it correct.
Your point is right.
I will take a look at the code although on my ipad I cannot experiment.
I pinpoint the problem and put this on my todo list on return from vacancy.
Tim,
In numerical/ question.php the tolerance for nominal for a small answer (i.e. 1e20) should not use the max function as the tiny fraction will be calculated with 1 and not the answer (ex 1e20).
This is the way the function was used in 1,9.
The ini_get('precision') is the limit of the digit part of the number, its real value is related to the exponent part of the number. This is why it is multiply by the answer value in the original code.
320 Â Â Â Â Â Â Â Â Â Â Â Â throwÂ newÂ coding_exception('CannotÂ workÂ outÂ toleranceÂ intervalÂ forÂ answerÂ *.'); 321 Â Â Â Â Â Â Â Â } 322 323 Â Â Â Â Â Â Â Â //Â WeÂ needÂ toÂ addÂ aÂ tinyÂ fractionÂ dependingÂ onÂ theÂ setÂ precisionÂ toÂ make 324 Â Â Â Â Â Â Â Â //Â theÂ comparisonÂ workÂ correctly,Â otherwiseÂ seeminglyÂ equalÂ valuesÂ can 325 Â Â Â Â Â Â Â Â //Â yieldÂ false.Â SeeÂ MDL3225. 326 Â Â Â Â Â Â Â Â $toleranceÂ =Â (float)Â $this>toleranceÂ +Â pow(10,Â 1Â *Â ini_get('precision')); 327 328 Â Â Â Â Â Â Â Â switchÂ ($this>tolerancetype)Â { 329 Â Â Â Â Â Â Â Â Â Â Â Â caseÂ 1:Â caseÂ 'relative': 330 Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â $rangeÂ =Â abs($this>answer)Â *Â $tolerance; 331 Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â returnÂ array($this>answerÂ Â $range,Â $this>answerÂ +Â $range); 332 333 Â Â Â Â Â Â Â Â Â Â Â Â caseÂ 2:Â caseÂ 'nominal': 334 Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â $toleranceÂ =Â $this>toleranceÂ +Â pow(10,Â 1Â *Â ini_get('precision'))Â * 335 Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â max(1,Â abs($this>answer)); 336 Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â returnÂ array($this>answerÂ Â $tolerance,Â $this>answerÂ +Â $tolerance); 337 338 Â Â Â Â Â Â Â Â Â Â Â Â caseÂ 3:Â caseÂ 'geometric': 339 Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â $quotientÂ =Â 1Â +Â abs($tolerance); 340 Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â returnÂ array($this>answerÂ /Â $quotient,Â $this>answerÂ *Â $quotient); 341 342 Â Â Â Â Â Â Â Â Â Â Â Â default: 343 Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â throwÂ newÂ coding_exception('UnknownÂ toleranceÂ typeÂ 'Â .Â $this>tolerancetype); 344 Â Â Â Â Â Â Â Â } 345 Â Â Â Â } 346
Yes, so the max(1, ...) is wrong. However, we need to deal correctly with the case where $this>answer is 0.
I Will do some tests on m'y iPad ... and give the results.
Tim,
I think we sould consider 0 a a special case as 0 is quite a special number.
Either 0 is an integer as could be the case in numerical question or a quite specific result of a calculation in calculated questions.
Either the tolerance to a 0 answer is 0 i.e. nothing else or 0 within the calculated precision (i.e. amount of money )
Using the tolerance parameter the user can define exactly what he wants or tolerates to the 0 answer.
The additional factor related to the PHP precision does not work for 0 PHP internal representation as for other numbers where the digital digits have different values related to the answer value.
Using the common formula for other answer values will give 0 to the tiny fraction a 0 value for answer== 0 and only the tolerance defined by the user (teacher) will be applied.
As an example on money calculations the user can define the tolerance as 0,00 i.e. cents.
Perhaps you could set a forum discussion on this if you think it's necessary.
I agree that 0 is a special case.
I don't currently have strong opinions about the 'right' way to handle it.
I think we should be able to find a way to handle it using the existing options on the question editing form that just works.
I will experiment the various combinations and use docs to help clarify the case.
Tim,
I put a first version of my analysis on the docs.
http://docs.moodle.org/dev/Question_Engine_2:Numerical_tolerances
There is only a very little discrepancy between my actual proposal on the docs and the 1,9 version in 0 answer case that could result from a calculation formula.
This discrepancy is perhaps just apparent as it implies the internal PHP rounding code for 0 result.
However the 1,9 code which inherits from preceeding versions, has been thoroughly tested.
I think we should first put a 1,9 equivalent code back and create the tests later.( perhaps create a separate issue)
There is a lot of 1,9 > 2,3 migration for next fall session that could justify to do it this way.
In any case, given the actual discussion, this code will not be redone in a near future...
Thank you for the excellent comparison. http://docs.moodle.org/dev/Question_Engine_2:Numerical_tolerances#Tests_results makes it very easy to compare the various implementations.
I have to say, however, I think Tim's proposal is the best. Look at the top row, and compare it to the second row. I think that the interval for tolerance 0 should be smaller than the interval for tolerance 1e24. In all the other cases your proposal and mine are both very close to the 1.9 implementation, which is definitely better than the 2.0 code.
Effectively it should be lower.
However when applied to 0.0, I think that the PHP code is working with a rounding that use the $epsilon i.e. 1e14 as the 0.O use all the mantissa to define itself as different from any lower value to the 1e300 limit.
Say differently 0  1e24 is not different than 0 +1e24 when applied to 0.0 so you are not testing a real MIN MAX.
I will do some tests but first the breakfast
Tim and Pierre, thanks for all your hard work on this. I keep looking at all this stuff with great interest. I wish I could help... I appreciate the thought and effort on this.
Tim,
The PHP rounding for 0 as answer is different following the way the result is obtained.
http://docs.moodle.org/dev/Question_Engine_2:Numerical_tolerances#HOW_PHP_handle_0
There seems to be no clear way to handle the 0 case.
The solution should follow the policy behind the epsilon calculation of grading the answer without conflict from the students.
It don't understand the point you are trying to make there.
I cannot imagine any real question where we would want the student to compute 11.0000000000000001 or 11.000000000000001. If you are interested in those sorts of numbers, then double precision is not good enough.
In any calculated question, we expect the teacher to set a sensible precision themselves, which is likely to involve only a few significant figures or decimal places.
There was a real problem with answers around 10^20, which we now have a proposed solution for. Unless I am missing something, I think that is good enough for all practical purposes.
I can imagine the case (pH calculation) where the calculated answer is 0, even more simply in money problems involving the EURO debts calculated to cents involving a precision greater than the $epsilon.
I imagine with more difficulties that the teacher set a 0 tolerance for such an answer.
The case of answer == 0 with a nominal interval of 0 where the 0 as answer result from a formula involving high precision
is effectively more theoretical than real.
I put the comment on the docs to cover all aspects as this could be useful in the future.
I agree with your solution ( Tim's idea ) and given my limited disponibilities in the next weeks (courses are back after a 5 months student strike and I need to install the PHP testing ), perhaps you should continue on this bug.
Your question on docs
" In particular, what tests would you like to add that will fail with the current code, but which will pass once we have fixed this bug?
"
This code is used by numerical, calculated and calculatedsimple questiontypes, the last 2 questiontypes using all the diferent tolerance types.
The tests should create the different conditions related to the actual code structure and verify that the min and max values are the ones expected.
This is not evident as these min, max values are calculated using the precision limit.
However giving that the tests need to be built following the code flow, I think that they are not necessary if we just put a warning in the code to not change this code flow until the tests as done on the docs remain valid.
P.S. There was already such a warning in the 1,9 code i.e."See MDL3225.", so perhaps my proposal is not the thing todo
Thanks to your excellent investigation, it will now be easy to make a patch and submit it for integration, so I will do that.
I think the testcases to add are basically the ones you put in the table on the wiki, so I will use the examples there.
Patch made based on Pierre's research, and submitted 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
Thanks, thats been integrated into 22 (no test), 23 and master.
curious: not adding any docs_required tag as this is a correction, but i am wondering if there are docs describing these intricate calculations as an overview. (disclaimer: lazy, not searched)
ah this is creating warnings for 22 :
E_USER_WARNING: unexpected operator '+' in /Users/Shared/Jenkins/Home/git_repositories/MOODLE_22_STABLE/lib/evalmath/evalmath.class.php on line 440
E_USER_WARNING: expecting a closing bracket in /Users/Shared/Jenkins/Home/git_repositories/MOODLE_22_STABLE/lib/evalmath/evalmath.class.php on line 440
E_USER_WARNING: operator '^' lacks operand in /Users/Shared/Jenkins/Home/git_repositories/MOODLE_22_STABLE/lib/evalmath/evalmath.class.php on line 440
i'll need to sink deeper into this to understand them.. no idea why atm.
It is more likely that the problem is with the test question you made.
The Calculated question type evaluates equations typed in by the teacher. Those error messages look like an error in what the tester typed, not in the Moodle code.
I agree with Tim.
Even if the code that analyze the formula is an old one, it is not handling perfectly all the users errors. MDL26823 is an example of this.
I could supply some specific examples if necessary.
ah ok, i take it this won't cause bc problems then. all's well
/question/type/numerical/simpletest/testanswer.php line 49 in 22 needs to be updated  cause of failure for simpletest.
ps: please disregard previous comment's user warnings  they were totally irrelevant
somebody missed to backport tests changes to 22_STABLE yup, so the old test (now deleted in 23 and master) is breaking 22 simpletests.
Oh, Grrr! I will amend the 22_stable commit. Sorry for missing that.
OK, new commit added to https://github.com/timhunt/moodle/compare/MOODLE_22_STABLE...MDL31837_22
Tim,
When you set your test values as in
// Zero tol, nonzero answer. $answer = new qtype_numerical_answer(13, 1e20, 1.0, '', FORMAT_MOODLE, 0.0); $this>assertFalse($answer>within_tolerance(0.9999999e20)); $this>assertTrue($answer>within_tolerance(1e20)); $this>assertFalse($answer>within_tolerance(1.0000001e20)); // Nonzero tol, zero answer. $answer = new qtype_numerical_answer(13, 0.0, 1.0, '', FORMAT_MOODLE, 1e24); $this>assertFalse($answer>within_tolerance(2e24)); $this>assertTrue($answer>within_tolerance(1e24)); $this>assertTrue($answer>within_tolerance(0)); $this>assertTrue($answer>within_tolerance(1e24)); $this>assertFalse($answer>within_tolerance(2e24)); // Zero tol, zero answer. $answer = new qtype_numerical_answer(13, 0.0, 1.0, '', FORMAT_MOODLE, 1e24); $this>assertFalse($answer>within_tolerance(1e20)); $this>assertTrue($answer>within_tolerance(1e35)); $this>assertTrue($answer>within_tolerance(0)); $this>assertTrue($answer>within_tolerance(1e35)); $this>assertFalse($answer>within_tolerance(1e20));
the values should use epsilon so that they will fit in any PHP settings.
I think I disagree. I do not believe that PHP epsilon will ever be different from 1e14.
If that belief is wrong, I would like to know about it. Therefore, having some unit tests that will fail if the assumption is wrong seems like a good idea to me.
You are right as long as the numbers are using the "standard" IEE values.
from http://www.php.net/manual/en/language.types.float.php
The size of a float is platformdependent, although a maximum of ~1.8e308 with a
precision of roughly 14 decimal digits is a common value (the 64 bit IEEE format).
also http://en.wikipedia.org/wiki/Doubleprecision_floatingpoint_format
So your "wait and see" policy is a valid option.
The MOODLE_22_STABLE extra commit has been integrated, leading to passed simpletests, thanks!
Thanks Eloy. just noting here that 2a and 2b work fine here (for Raj)
Works Great
Thanks for fixing this Tim.
FYI: 3.b Values seems wrong, although tested with 1e5 => 0.00001 and works fine. Hope this is what is expected.
Check that 1e20 and 1.0000000001 are graded right (When tolerance is 0)
Pierre, any thoughts on when this might get fixed? I'm coming up on using those questions again in midSeptember in my class.