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.

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

[#12544] Rubric Question Statistics: Handle empty weights #12545

Merged

Conversation

jasonqiu212
Copy link
Contributor

@jasonqiu212 jasonqiu212 commented Aug 5, 2023

Fixes #12544

Changes:

  • Skip over null weight values when calculating average weight
  • Add explanation on usage of empty weights

Screenshots:

  • Empty weights are displayed as empty string, as shown in the screenshots below (i.e. [])

Viewing statistics:
Response Summary

  • In above screenshot, calculation of average skips over empty weights
    • (7 * 0 + 8 * 10) / 15 = 5.33

Per Recipient Stats (Per Criterion)

  • In above screenshot, calculation of average skips over empty weights
    • (1 * 0 + 1 * 10) / 2 = 5

Per Recipient Stats (Overall)

  • In above screenshot, calculation of average skips over empty weights
    • (2 * 0 + 2 * 10) / 4 = 5

Downloaded result:
Downloaded result

@github-actions
Copy link

github-actions bot commented Aug 5, 2023

Hi @jasonqiu212, thank you for your interest in contributing to TEAMMATES!
However, your PR does not appear to follow our contribution guidelines:

  • Title must start with the issue number the PR is fixing in square brackets, e.g. [#<issue-number>]

Please address the above before we proceed to review your PR.

@jasonqiu212 jasonqiu212 changed the title Rubric Question Statistics: Handle null weights [#12544] Rubric Question Statistics: Handle null weights Aug 5, 2023
@jasonqiu212 jasonqiu212 changed the title [#12544] Rubric Question Statistics: Handle null weights [#12544] Rubric Question Statistics: Handle empty weights Aug 5, 2023
@jasonqiu212 jasonqiu212 marked this pull request as ready for review August 5, 2023 14:51
@jasonqiu212 jasonqiu212 added the s.ToReview The PR is waiting for review(s) label Aug 7, 2023
@weiquu
Copy link
Contributor

weiquu commented Aug 8, 2023

Hi @jasonqiu212, I tried to leave the weight input box blank when creating a Rubric question, but am encountering a server error. Can I check if you're facing the error as well? Not sure if there's an issue with my setup. Here's the input (with the error):

Screenshot 2023-08-08 at 4 45 59 PM

Snippet of stack trace:

java.lang.NumberFormatException: empty String
        at java.base/jdk.internal.math.FloatingDecimal.readJavaFormatString(FloatingDecimal.java:1842)
        at java.base/jdk.internal.math.FloatingDecimal.parseDouble(FloatingDecimal.java:110)
        at java.base/java.lang.Double.parseDouble(Double.java:543)

@jasonqiu212
Copy link
Contributor Author

jasonqiu212 commented Aug 8, 2023

@weiquu, Thanks for flagging the issue! I believe it's due to the conversion between null and NO_VALUE. I reverted the empty input to be represented by null. It should work fine now.

@jasonqiu212 jasonqiu212 force-pushed the 12544-fix-rubric-question-null-weight branch from ae07ebf to 4b6d504 Compare August 8, 2023 12:24
@weiquu
Copy link
Contributor

weiquu commented Aug 10, 2023

Thanks @jasonqiu212 for the changes! Just one more thing:
Screenshot 2023-08-10 at 11 22 05 PM

For this question, I set all options to have empty weights. I noted two possible areas of improvement:

  1. The "Average" (and "Total") columns for Response Summary and Per Recipient Statistics (Per Criterion) should probably indicate that the average is not calculable, given that the answers all have empty weights. I would suggest indicating something like -. What do you think?
  2. For the Per Recipient Statistics (Overall), the "Total" and "Per Criterion Average" columns should follow the same indication as above. In addition, the "Average" column should also show the same thing, instead of the NaN it shows now

@jasonqiu212
Copy link
Contributor Author

@weiquu Yep, I agree! I will work on adding this indication of non-calculable averages (-). Thanks for pointing out this edge case!

@jasonqiu212
Copy link
Contributor Author

As suggested by @weiquu, I display - to indicate that the aggregate value cannot be calculated when all values are empty.

Screen Shot 2023-08-15 at 12 23 04 AM

Copy link
Contributor

@weiquu weiquu left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the fix

@weiquu weiquu added s.FinalReview The PR is ready for final review and removed s.ToReview The PR is waiting for review(s) labels Aug 18, 2023
@domlimm
Copy link
Contributor

domlimm commented Aug 19, 2023

@jasonqiu212 @weiquu will get to this today

Copy link
Contributor

@domlimm domlimm left a comment

Choose a reason for hiding this comment

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

LGTM! Very nice job @jasonqiu212! Thank you for fixing this 💯

@weiquu weiquu removed the request for review from cedricongjh August 19, 2023 07:55
@weiquu weiquu added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.FinalReview The PR is ready for final review labels Aug 19, 2023
@weiquu weiquu merged commit b49359e into TEAMMATES:master Aug 19, 2023
9 checks passed
@damithc
Copy link
Contributor

damithc commented Aug 20, 2023

@samuelfangjw shall we do a new release with this fix?

@samuelfangjw samuelfangjw added this to the V8.29.0 milestone Aug 21, 2023
@samuelfangjw samuelfangjw added the c.Bug Bug/defect report label Aug 21, 2023
@damithc
Copy link
Contributor

damithc commented Aug 21, 2023

Fix deployed. Thanks for the fix @jasonqiu212 and @samuelfangjw @domlimm @weiquu for inputs/reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c.Bug Bug/defect report s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot download results of session due to
5 participants