Display % of points granted by the checker when partially scoring #301
Conversation
There was a problem hiding this comment.
Hey, thank you for delivering a PR. Did You tested the translations?
I tried testing it and it seems that when You change user's language the comment is not reflected.
I believe that, this string "Granted...." is saved during judging of the problem and there is no access to user's language preferences there.
I might have clicked something wrong though.
oioioi/programs/handlers.py
Outdated
| @@ -555,6 +555,9 @@ def make_report(env, kind='NORMAL', save_scores=True, **kwargs): | |||
| test_report.score = result['score'] if save_scores else None | |||
| test_report.status = result['status'] | |||
| test_report.time_used = result['time_used'] | |||
| percentage = result.get('result_percentage', 0.) | |||
| if percentage != 100. and percentage != 0.: | |||
There was a problem hiding this comment.
Can You please convince me it is okay here to check equality of some floating point number? I would prefer to rather see
if percentage > 0 and percentage < 100:
keep in mind that sometimes "different" number are equal due to precision
>>> 100.000000000000001 == 100.
True
>>> 100.00000000000001 == 100.
False
There was a problem hiding this comment.
Also, the domain of these values is not necessarily [0, 100]. I actually have abused this mechanism with values well above 100 to circumvent some architectural shortcomings once.
There was a problem hiding this comment.
Thanks for the insight. In the wiki it says that the percentage is a whole integer. It says that this option can be used to give partial scoring. It does not mention whether it is allowed to grant more than 100%. It should be more documented in the specs
As for this logic, I am not sure we should tell user that he got 150% for this test, as this i probably not the intended behavior for this feature and was properly called "an abuse".
I suspect there would be disagreements about this, so please express your opinions here!
There was a problem hiding this comment.
I don't know if the percentage value should be greater than 100, but in the context of this PR I think that the message should not be displayed when the value is 0 (then the result should be WA) or 100 (then there is no comment and you can see assume that you have full points).
But if we decided to allow a percentage greater than 100, we would have to find another way to check whether this value is equal to 100
There was a problem hiding this comment.
First of all, if we could change this to an Integer, the float-related discussion is unnecessary. Is there any reason to use FloatField not IntegerField?
Secondly I think that Verdict and partial scoring are different things and it should be allowed to give 0% points but OK as a verdict. The checker returns both the verdict and partial scoring independently. We should treat is as such independent values.
Thirdly If partial scoring does not affect number of points awarded there should be no comment (what for it would be). If user get less points we definitely should post the comment. If it is allowed to give more, it is debatable if it should be displayed.
| {% if test.comment %} | ||
| {% if test.result_percentage %} | ||
| {% blocktranslate with test.result_percentage as percentage %} | ||
| Granted {{ percentage }}% points, comment: |
There was a problem hiding this comment.
How about "Granted {{ percentage }}% of points". Also as percentage could be 33.33333333 I think we should pretty print this floating point value
| @@ -24,7 +24,22 @@ | |||
| {% endif %} | |||
| {% endif %} | |||
| {% if allow_test_comments %} | |||
| <span>{{ test.comment }}</span> | |||
| <span> | |||
| {% if test.comment %} | |||
There was a problem hiding this comment.
This logic is quite complex. Do we have any performance degradation due to this change?
I also disagree that the percentage should be graphically the same as comment. They should be visually different.
There was a problem hiding this comment.
@DietPawel do you still request changes?
There was a problem hiding this comment.
There are 3 things I am still not sure about:
- math.isclose instead of
==for Floats. - pretty-printing of Floats
- Performance of django templates is not ideal. Is there any visible performance penalty for this template expansion?
Other than that I am totally for.
There was a problem hiding this comment.
result_percentageis now aFraction, so there are no rounding errors- added
- for ~60 tests each with comment and result percentage, the templates render less than 0.01s slower
DietPawel
changed the title
DietPawel
changed the title
Addition of translation is required
Closes #288