-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: increase grading rounding precision to avoid incorrect grades #27788
fix: increase grading rounding precision to avoid incorrect grades #27788
Conversation
Thanks for the pull request, @Agrendalath! What's next?Please work through the following steps to get your changes ready for engineering review: 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. 🔘 Let us know that your PR is ready for review:Who will review my changes?This repository is currently maintained by Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:
When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
f296b60
to
f8ef896
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 @Agrendalath LGTM.
- I tested this: Scores are displayed with more precision on Progress page graph.
- I read through the code
- I checked for accessibility issues
- Includes documentation
- N/A
I made sure any change in configuration variables is reflected in the corresponding client'sconfiguration-secure
repository.
@natabene, this is ready for your review. |
@Agrendalath Thank you for your contribution. |
📣 💥 Heads-up: You must either rebase onto master or merge master into your branch to avoid breaking the build. We recently removed diff-quality and introduced lint-amnesty. This means that the automated quality check that has run on your branch doesn't work the same way it will on master. If you have introduced any quality failures, they might pass on the PR but then break the build on master. This branch has been detected to not have commit 2e33565 as an ancestor. Here's how to see for yourself:
If you have any questions, please reach out to the Architecture team (either #edx-shared-architecture on Open edX Slack or #architecture on edX internal). |
f8ef896
to
ebc2adf
Compare
ebc2adf
to
16b88f9
Compare
Your PR has finished running tests. There were no failures. |
16b88f9
to
6c60390
Compare
Hello @natabene, would this change require a product review or is it suitable for a core contributor review? |
@nizarmah Let me check and get back to you asap. |
@nizarmah Thanks for asking, we considered this and will need to do a product review first. |
@natabene, just checking on this PR. |
Sorry, no news yet. |
6c60390
to
55b50ea
Compare
55b50ea
to
316fef4
Compare
Hey @Agrendalath - I'm the new 2U Product Manager working on grading. Apologies on the delay in response here. Can you confirm my understanding of this change? It appears that this will increase the accuracy, of the rounding that is already in place, up to 4 decimal places. Is that correct? are there any other changes that I'm not catching? |
Hi @ProductRyan, thank you for checking. That's correct - the subsection grades calculated by the platform are inaccurate because of the double rounding. Therefore, we want to increase the rounding precision to produce correct results. This change will not affect existing persistent grades as long as they are not re-generated. Otherwise, this can cause up to a 1 percentage point difference in the results (on a rare occasion, like the one described in the PR). |
@ProductRyan, @natabene, just a friendly reminder that this is still waiting for the product review. |
19ac721
to
80f3177
Compare
@Agrendalath I'm going through the old PRs, do you still want to pursue this PR or should we close this for now? If you're still interested I can try to get some eyes on this that could review this for you. |
@feanil, thank you for checking. We're definitely interested in merging this. Please see my last comment in the product review thread. |
Sounds good , thanks I'll watch the product review thread and see what I can do to help unblock that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving since Jenna has signed off on this from the product side: openedx/platform-roadmap#238 (comment)
We used two decimal digits to match the experience from the edx-platform. However, openedx/edx-platform#27788 increased the precision to reduce the impact of double rounding.
Enabling the rounding in openedx#16837 has been causing noticeable (up to 1 percentage point) differences between non-rounded subsection grades and a total grade for a course. This increases the grade precision to reduce the negative implications of double rounding.
80f3177
to
aabc594
Compare
Update: I'll merge this once openedx/frontend-app-learning#1397 is approved. |
We used two decimal digits to match the experience from the edx-platform. However, openedx/edx-platform#27788 increased the precision to reduce the impact of double rounding.
We used two decimal digits to match the experience from the edx-platform. However, openedx/edx-platform#27788 increased the precision to reduce the impact of double rounding. (cherry picked from commit e4a0105)
We used two decimal digits to match the experience from the edx-platform. However, openedx/edx-platform#27788 increased the precision to reduce the impact of double rounding. (cherry picked from commit e4a0105)
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
2U Release Notice: This PR has been deployed to the edX production environment. |
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
2U Release Notice: This PR has been deployed to the edX production environment. |
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
2U Release Notice: This PR has been deployed to the edX production environment. |
We used two decimal digits to match the experience from the edx-platform. However, openedx/edx-platform#27788 increased the precision to reduce the impact of double rounding. (cherry picked from commit e4a0105)
We used two decimal digits to match the experience from the edx-platform. However, openedx/edx-platform#27788 increased the precision to reduce the impact of double rounding. (cherry picked from commit e4a0105)
Enabling the rounding in #16837 has been causing noticeable (up to 1 percentage point) differences between non-rounded subsection grades and a total grade for a course. This increases the grade precision to reduce the negative implications of double rounding.
Jira
OSPR-5819
Sandbox
https://pr27788.sandbox.opencraft.hosting/
Direct link to the progress page (you can log in as staff).
Testing instructions
Progress
page and see that the score without this PR is 35%. After this change, it should be 34%.Explanation
This course contains two subsections graded as
Homework
(weight: 75%):In the current approach, the grades of subsections are rounded. Therefore, in this case, we're getting
(67% + 25%) / 2 / 4 * 3
(/ 4 * 3
is because of the weight (75%)), which gives us34.5
. This is rounded up to35%
.This PR changes these calculations to
(66.67% + 25%) / 2 / 4 * 3
, which returns34.37625
, which is then rounded down to34%
.Deadline
None.
Reviewers
Other information
Private-ref: BB-4210
Product Review: openedx/platform-roadmap#238
Settings
Tutor requirements