Skip to content

MCKIN-12690 Fix Score rounding - #267

Merged
xitij2000 merged 3 commits into
open-craft:masterfrom
moeez96:MCKIN-12690
Mar 30, 2020
Merged

MCKIN-12690 Fix Score rounding#267
xitij2000 merged 3 commits into
open-craft:masterfrom
moeez96:MCKIN-12690

Conversation

@moeez96

@moeez96 moeez96 commented Mar 9, 2020

Copy link
Copy Markdown
Contributor

Jira ticket: https://edx-wiki.atlassian.net/browse/MCKIN-12690
Python 3:
Decimal(0.485) = 0.4849999
Decimal(48.5) = 48.5

Due to above mentioned, rounding is in-accurate.
We need to multiply score by 100 before giving it as input to Decimal(xxx).

@moeez96

moeez96 commented Mar 9, 2020

Copy link
Copy Markdown
Contributor Author

@xitij2000 Please review, merge and tag.

@xitij2000

Copy link
Copy Markdown
Member

@moeez96 Could you look into the failing tests?

@moeez96

moeez96 commented Mar 10, 2020

Copy link
Copy Markdown
Contributor Author

@xitij2000 Sure. failing tests seem like missing plugins. Does not looks like code failure

@xitij2000

Copy link
Copy Markdown
Member

@xitij2000 Sure. failing tests seem like missing plugins. Does not looks like code failure

I understand, but these same tests were passing before. I'm not saying this PR introduced the failures, just that each PR needs to maintain the current state of the code, so should leave the tests in a passing condition.

@petarmaric

petarmaric commented Mar 18, 2020

Copy link
Copy Markdown
  • I tested this:
    • as per the projects testing instructions. I was also unable to reproduce the failing CircleCI tests locally.
    • I personally wouldn't mix floats and Decimals together as that can be a source of fairly subtle and nasty bugs, but my Minimal Reproducible Example (MRE) below shows that his approach does have its merits:
$ python3.6 -c 'from decimal import *; print("\n".join("%.16e %7.3f %3d %3d" % (x, x, int((Decimal(x) * 100).quantize(Decimal('1.'), rounding=ROUND_HALF_UP)), int(Decimal(x*100).quantize(Decimal('1.'), rounding=ROUND_HALF_UP))) for x in [0.474, 0.475, 0.476, 0.484, 0.485, 0.486, 0.494, 0.495, 0.496]))'
4.7399999999999998e-01   0.474  47  47
4.7499999999999998e-01   0.475  47  48  # old 47, PR 48
4.7599999999999998e-01   0.476  48  48
4.8399999999999999e-01   0.484  48  48
4.8499999999999999e-01   0.485  48  49  # old 48, PR 49
4.8599999999999999e-01   0.486  49  49
4.9399999999999999e-01   0.494  49  49
4.9500000000000000e-01   0.495  49  50  # old 49, PR 50
4.9600000000000000e-01   0.496  50  50
  • I read through the code
  • I checked for accessibility issues
  • Includes documentation

This is a provisional 👍

Please resolve the following and then merge the PR:

  1. Fix failing CI checks for Python 2.7. I suggest the PR author to rerun the CI checks, to check if this was a one-off CircleCI fluke.
  2. Bump the Python package to a new bugfix version, git tag it, and perform a new PyPI release.

@moeez96

moeez96 commented Mar 24, 2020

Copy link
Copy Markdown
Contributor Author

@petarmaric I am unable to find the cause of failing tests. Also, FYI I do not have access to merge or create release tag.

@petarmaric

Copy link
Copy Markdown

@xitij2000 Any thoughts on this ^

@xitij2000

Copy link
Copy Markdown
Member

@moeez96 I'm unable to find a cause either. I'll create a separate ticket for this, and merge the PR as-is for now.

@xitij2000
xitij2000 merged commit 4e5e723 into open-craft:master Mar 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants