Skip to content
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.

Sign up for GitHub

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

[Bugfix:RainbowGrades] Correct Handle of ExtraCredit #74

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[Bugfix:RainbowGrades] Correct Handle of ExtraCredit #74

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions gradeable.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()).

return extracreditmap.find(id)->second;
}
float getItemMaximum(const std::string &id) const {
if (maximums.find(id) == maximums.end()){
return 0;
Expand Down Expand Up @@ -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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really want it to break if setExtraCredit() is called with gradeable id that is already set as extra credit?

extracreditmap[id] = is_extracredit;
}

void setMaximum(const std::string&id, float maximum) {
assert (hasCorrespondence(id));
assert (maximums.find(id) == maximums.end());
Expand Down Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

std::map<std::string,std::string> original_ids;
std::map<std::string,std::string> resubmit_ids;
std::map<std::string,float> autograde_replacement_percentages;
Expand Down
5 changes: 5 additions & 0 deletions main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

float maximum = grade_id.value("max",0.0);
GRADEABLES[g].setMaximum(token_key,maximum);

Expand Down
44 changes: 41 additions & 3 deletions student.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,11 +127,12 @@ void Student::setGradeableItemGrade_border(GRADEABLE_ENUM g, int i, float value,

class score_object {
public:
score_object(float s,float m,float p,float sm):score(s),max(m),percentage(p),scale_max(sm){}
score_object(float s,float m,float p,float sm,bool ec):score(s),max(m),percentage(p),scale_max(sm),extra_credit(ec){}
float score;
float max;
float percentage;
float scale_max;
bool extra_credit;
};

bool operator<(const score_object &a, const score_object &b) {
Expand All @@ -140,11 +141,13 @@ bool operator<(const score_object &a, const score_object &b) {
float p1 = a.percentage;
float sm1 = a.scale_max;
float my_max1 = std::max(m1,sm1);
bool ec1 = a.extra_credit;
float s2 = b.score;
float m2 = b.max;
float p2 = b.percentage;
float sm2 = b.scale_max;
float my_max2 = std::max(m2,sm2);
bool ec2 = b.extra_credit;
// Grades should be compared by the normalized grade only, not normalized grade multiplied by percentages. Otherwise, a grade of 90 for the gradeable
// with percentage 0.1 will always be considered lower than a grade of 60 for the gradeable with percentage 0.2, since 0.1 * 0.9 < 0.2 * 0.6
bool result;
Expand Down Expand Up @@ -206,28 +209,44 @@ float Student::GradeablePercent(GRADEABLE_ENUM g) const {

// collect the scores in a vector
std::vector<score_object> scores;
std::vector<score_object> only_ec;

for (int i = 0; i < GRADEABLES[g].getCount(); i++) {
float s = getGradeableItemGrade(g,i).getValue();
std::string id = GRADEABLES[g].getID(i);
float m = nonzero_sum/nonzero_count;
//if(!id.empty() && GRADEABLES[g].isReleased(id)){
if(!id.empty()){
m = GRADEABLES[g].getItemMaximum(id);
// std::cout << "m" << m << std::endl;
// std::cout << "m" << m << std::endl;
}
bool ec = GRADEABLES[g].isExtraCredit(id);
float p = GRADEABLES[g].getItemPercentage(id);
float sm = GRADEABLES[g].getScaleMaximum(id);
scores.push_back(score_object(s,m,p,sm));
if(!ec) {
scores.push_back(score_object(s,m,p,sm,ec));
} else {
only_ec.push_back(score_object(s,m,p,sm,ec));
}
}

// sort the scores (smallest first)

std::sort(scores.begin(),scores.end());
//to check that the number of "drop the lowest" is less than the number of non extra credit gradeables,
// i.e., it is not allowed to drop all non extra credit gradeables
assert (GRADEABLES[g].getRemoveLowest() >= 0 && (
(non_extra_credit_count > 0 && GRADEABLES[g].getRemoveLowest() < non_extra_credit_count)) ||
(GRADEABLES[g].getRemoveLowest() == 0));

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";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

exit(1);
}


// sum the remaining (higher) scores
float sum_max = 0;
for (int i = GRADEABLES[g].getRemoveLowest(); i < GRADEABLES[g].getCount(); i++) {
Expand All @@ -253,6 +272,7 @@ float Student::GradeablePercent(GRADEABLE_ENUM g) const {
float m = scores[i].max;
float p = scores[i].percentage;
float sm = scores[i].scale_max;
bool ec = scores[i].extra_credit;
float my_max = std::max(m,sm);

if (p < 0) {
Expand All @@ -274,6 +294,24 @@ float Student::GradeablePercent(GRADEABLE_ENUM g) const {
sum_percentage += p;
}
}



for (int i = 0; i < only_ec.size(); i++) {
float ec_s = only_ec[i].score;
float ec_m = only_ec[i].max;
float ec_p = only_ec[i].percentage;
float ec_sm = only_ec[i].scale_max;
bool ec_ec = only_ec[i].extra_credit;
float my_max = std::max(ec_m,ec_sm);

sum += ec_p * ec_s / my_max;



}


const float tolerance = 0.000001;
assert(sum_percentage <= 1.0 + tolerance);
if (sum_max == 0) { // pure extra credit category
Expand Down
Loading