[Bugfix:RainbowGrades] Correct Handle of ExtraCredit #74
Conversation
ziesski
changed the title
There was a problem hiding this comment.
I believe this PR mostly solves the immediate problem it was supposed to solve. There are minor refactorings that I think could be applied though but they are not critical.
A more important issue is the incorrect computation of grade thresholds. It easiest to see with the "perfect" grade being above 100%:
Perfect score at the end of the semester must be exactly 100% (screenshot from the correct version of code):
even when there are extra credit gradeables.
All other thresholds are also incorrect.
At the same time, a much more important concern is the overall approach to specifying extra credit items and pure extra credit categories in customization.json. To bring some context, previously the "extra_credit" parameter has been introduced very long time ago and I believe it was used at some point. When I was browsing some very old examples, I saw it there, and I started using it in my own configurations.
Then I learned that in at least the last five years, this "extra_credit" was not actually parsed, so it was just completely ignored by the Rainbow Grades code. At that time I was told that to deduce if a gradeable is extra credit, Rainbow Grades would look at the "max" and "scale_max" parameters. Regular gradeables would have "scale_max" of 0 and "max" as the number of points the gradeable is worth. Extra credit gradeables would be the opposite: "max" of 0 and "scale_max" of whatever the extra credit this gradeable is worth.
The problem back then was that Rainbow Grades code was not doing the drop the lowest feature correctly when extra credit items were present. So, I came up with some patches that were addressing it.
Now, with this PR re-introducing the "extra_credit" parameter, I am convinced that we need to discuss how the extra credit should be properly specified, so that it is clear to the users and easy to use. If we leave all three parameters: "extra_credit", "max", and "scale_max", it would lead to confusion. Especially given that those features are not properly documented, our users would not know how to use these three parameters correctly. It was confusing when there were just two: "max", and "scale_max". Now that there are three after we brought "extra_credit" back, we need to decide what different combinations of these three parameters should represent. Without having this conversation and without making those decisions, the "drop the lowest" implementation will continue to be a constant headache for the users and Submitty developers alike.
| @@ -181,6 +185,12 @@ class Gradeable { | |||
| released[id] = is_released; | |||
| } | |||
|
|
|||
| void setExtraCredit(const std::string&id, bool is_extracredit) { | |||
| assert (hasCorrespondence(id)); | |||
| assert (extracreditmap.find(id) == extracreditmap.end()); | |||
There was a problem hiding this comment.
Do we really want it to break if setExtraCredit() is called with gradeable id that is already set as extra credit?
| @@ -647,6 +647,11 @@ void preprocesscustomizationfile(const std::string &now_string, | |||
| GRADEABLES[g].setReleased(token_key,released); | |||
| } | |||
|
|
|||
| bool extra_credit = grade_id.value("extra_credit", false); | |||
| if (extra_credit) { | |||
| GRADEABLES[g].setExtraCredit(token_key,extra_credit); | |||
There was a problem hiding this comment.
So, we will only be adding gradeables that are extra credit to this map, right? But then it implies the invariant that says that all entries must have true as the value. See my other comment about the invariant.
| @@ -230,6 +240,7 @@ class Gradeable { | |||
| std::vector<float> sorted_weights; | |||
| std::map<std::string,float> clamps; | |||
| std::map<std::string,bool> released; | |||
| std::map<std::string,bool> extracreditmap; | |||
There was a problem hiding this comment.
What is the extracreditmap's invariant? Is it meant to be "For all entries in the map, the value is true for any key"? But then what is the purpose of specifically using a map? We could have gone with a vector which would make it much easier to enforce the invariant then.
It appears to me that a better solution would be to keep extracreditmap as a map but to add all gradeables to it, like it happens with "released" map. Then extra credit gradables would have the value of true, and others - false.
| @@ -119,6 +119,10 @@ class Gradeable { | |||
| assert (released.find(id) != released.end()); | |||
| return released.find(id)->second; | |||
| } | |||
| bool isExtraCredit(const std::string &id) const { | |||
| // assert (extracreditmap.find(id) != extracreditmap.end()); | |||
There was a problem hiding this comment.
Having:
if (extracreditmap.find(id) == extracreditmap.end()){
return false;
}
would make the code more readable and more similar to how other code is written (see, e.g., getItemMaximum()).
| if (!( ((GRADEABLES[g].getRemoveLowest() >= 0) && ( | ||
| (non_extra_credit_count > 0 && GRADEABLES[g].getRemoveLowest() < non_extra_credit_count))) || | ||
| (GRADEABLES[g].getRemoveLowest() == 0))) { | ||
| std::cerr << "Error: Invalid value for removeLowest in gradeable Check the conditions.\n"; |
There was a problem hiding this comment.
The parameter is called "remove_lowest", so the error message should say std::cerr << "ERROR: Invalid value for remove_lowest in Rainbow Grades configuration. Check customization.json." << std::endl;
There was a problem hiding this comment.
I believe this PR mostly solves the immediate problem it was supposed to solve. There are minor refactorings that I think could be applied though but they are not critical.
A more important issue is the incorrect computation of grade thresholds. It easiest to see with the "perfect" grade being above 100%:
Perfect score at the end of the semester must be exactly 100% (screenshot from the correct version of code):
even when there are extra credit gradeables.
At the same time, a much more important concern is the overall approach to specifying extra credit items and pure extra credit categories in customization.json. To bring some context, previously the "extra_credit" parameter has been introduced very long time ago and I believe it was used at some point. When I was browsing some very old examples, I saw it there, and I started using it in my own configurations.
Then I learned that in at least the last five years, this "extra_credit" was not actually parsed, so it was just completely ignored by the Rainbow Grades code. At that time I was told that to deduce if a gradeable is extra credit, Rainbow Grades would look at the "max" and "scale_max" parameters. Regular gradeables would have "scale_max" of 0 and "max" as the number of points the gradeable is worth. Extra credit gradeables would be the opposite: "max" of 0 and "scale_max" of whatever the extra credit this gradeable is worth.
The problem back then was that Rainbow Grades code was not doing the drop the lowest feature correctly when extra credit items were present. So, I came up with some patches that were addressing it.
Now, with this PR re-introducing the "extra_credit" parameter, I am convinced that we need to discuss how the extra credit should be properly specified, so that it is clear to the users and easy to use. If we leave all three parameters: "extra_credit", "max", and "scale_max", it would lead to confusion. Especially given that those features are not properly documented, our users would not know how to use these three parameters correctly. It was confusing when there were just two: "max", and "scale_max". Now that there are three after we brought "extra_credit" back, we need to decide what different combinations of these three parameters should represent. Without having this conversation and without making those decisions, the "drop the lowest" implementation will continue to be a constant headache for the users and Submitty developers alike.
Current Submitty has issue with remove lowest and extra credit homework.
Suppose a student has the following homework grades (each has max 50 points):
42 45 40 44 46 48 49 50 35 (the last one is extra credit HW9)
Following the same criteria of dropping the lowest 2 homework assignments and considering the last one as optional extra credit, the computation should yield:
Currently, Submitty drop the lowest 2 grades: 40 and 35 are dropped.
However, Last one is extra credit so should not get dropped, instead 40, 42 should be dropped.
This patch fixes above error. Below is example of above mentioned scenario.