Tolerance expressed in percentage now computes correctly. [BLD-522]

[jmclaus]

@auraz @valera-rozuvan It fixes the issue, tested it with -100 +- 10%; 100 +- 10%; 0 +- 10%; 10 +- 100%; -10 +- 100%. I guess we should add tests.

[singingwolfboy]

Adding tests is always good!

[jmclaus]

@auraz @valera-rozuvan @singingwolfboy

This is an example of a test that did not pick up an error in the source code. Here are the details:

The erroneous code in src was (with complex1 : student result; complex2 : instructor result):

tolerance = tolerance * max(abs(complex1), abs(complex2))

In the original tests, the answer was 4.0 and the tolerance 10%. The range of correct answers is then [3.6, 4.4].

But 4.44 would generate a correct answer as

tolerance = 0.1 * max(4, 4.44) = 0.444 and the erroneous range of correct answers was set to [3.556, 4.444].

The tests would only check 4.5 and not a value closer to the bounds of correctness. I modified this but arbitrarily test 4.4000001 for incorrectness. Should something even tighter be put in place? In that case, what do you think it should be?

[auraz]

@jmclaus this is not copy-paste error. This code was originally there:
https://github.com/edx/edx-platform/blob/rc/2013-12-03/common/lib/capa/capa/util.py

Please also fix https://github.com/edx/edx-platform/blob/master/common/lib/capa/capa/util.py#L33, as it is copy paste from https://github.com/edx/edx-platform/blob/master/common/lib/capa/capa/util.py#L36

[auraz]

Also, I suggest to rename complex1 and complex2 to student_complex and teacher_complex or like that.

[auraz]

Regarding tighter tests: I can't imagine what needs to be tested tighter right now.

[auraz]

Why zero is in incorrect responses?

[jmclaus]

@auraz No reason, removed.

[auraz]

@jmclaus for some reason Jenkins tests are not run for this PR.

[jmclaus]

@auraz Also, I suggest to rename complex1 and complex2 to student_complex and teacher_complex or like that. --> Renamed complex1 and complex2 to student_complex and instructor_complex.

[jmclaus]

@auraz Regarding tighter tests: I can't imagine what needs to be tested tighter right now. --> OK, I'll leave the new tests as is.

[jmclaus]

@auraz Please also fix https://github.com/edx/edx-platform/blob/master/common/lib/capa/capa/util.py#L33, as it is copy paste from https://github.com/edx/edx-platform/blob/master/common/lib/capa/capa/util.py#L36

Fixed.

[jmclaus]

@auraz Seems like a lot of current PR's do not have Jenkin tests running.

[auraz]

@jmclaus do you know how to run manual build?

[jmclaus]

@olmar (since @alex is out for the week), @valera-rozuvan @polesye The tests now all pass locally, please review.

The initial fix I did solved BLD-522 but was breaking other things. compare_with_tolerance is used throughout capa/capa/responsetype.py with a default_tolerance set to '0.001%' and expects it to be relative [tolerance * max(abs(student_complex), abs(instructor_complex))]. But when an instructor uses, for example, 4 +- 10% in Studio, he expects it to be absolute [tolerance = tolerance * abs(instructor_complex))]

See http://randomascii.wordpress.com/2012/02/25/comparing-floating-point-numbers-2012-edition/

[alex]

I am not the @alex you are looking for :-)

[jmclaus]

@alex Indeed, sorry! It was @auraz.

[olmar]

@jmclaus I reviewed and tested your pr manually also. It is OK for issue described in ticket.

[olmar]

but not sure about your concerns what expects instructor in studio. @valera-rozuvan please review also.

[jmclaus]

@olmar Thanks for review. Sorry, I wasn't clear enough. This fix actually gets the instructor what he expects (so no concern at all), 4 +- 10% will now give the following range of correct answers: [3.6, 4.4].

[olmar]

👍

[polesye]

@auraz will finish code review on Monday.

[auraz]

Do we have not-a-string-type tolerance anywhere?

[auraz]

O, I've found one place.

[auraz]

In previous code, when tolerance was not ending on '%', it was going through 'evaluator(dict(), dict(), tolerance)'. Now it is going through only if it is string. Why it is so?

[jmclaus]

@auraz Is there any reason to evaluate anything else than a string?

[auraz]

I do not know, It should be investigated.

[jmclaus]

@auraz compare_with_tolerance is used in only 1 file responsetypes.py 5 times (if we don't count tests). It's value is either float_info.epsilon, the default value (0.001%) or what is specified in the XML. Do you think this might need evaluation? I guess to be on the safe side, we should let it go through evaluator(dict(), dict(), tolerance) every time it's not a percentage. Thanks. Will do.

[jmclaus]

@auraz I take this back, if you pass a float through the evaluator, the following error is raised:

Traceback (most recent call last):
    common/lib/capa/capa/tests/test_util.py line 44 in test_compare_with_tolerance
      result = compare_with_tolerance(109.9, 100.0, 10.0, False)
    common/lib/capa/capa/util.py line 37 in compare_with_tolerance
      tolerance = evaluator(dict(), dict(), tolerance)
    common/lib/calc/calc/calc.py line 228 in evaluator
      if math_expr.strip() == "":
   AttributeError: 'float' object has no attribute 'strip'

I propose we leave things as is. I tested the code manually and tests all pass. What do you think?

[auraz]

thank you for looking into. I agree with you.

[auraz]

I suggest to also add small test especially for compare_with_tolerance function, because we have no tests for float tolerances.

[jmclaus]

@auraz I added unit tests for compare_with_tolerance to test_response_types.py. Or should we create a new file called test_util.py and put these in there?

[auraz]

@jmclaus new file is better decision.

[jmclaus]

@auraz I put the tests in a new file called test_util.py. They run and pass locally.

[jmclaus]

@auraz I have addressed all your comments then. Please finish review. Thanks.

[jmclaus]

@auraz Good to merge then? Tests all back to green.

[auraz]

👍