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

Jump to bottom

feat: overlay subject from dynamic score #91

Merged
merged 4 commits into from
Jan 20, 2025
Merged

feat: overlay subject from dynamic score #91

merged 4 commits into from
Jan 20, 2025

Conversation

hey2022
Copy link
Owner

@hey2022 hey2022 commented Jan 12, 2025 •
edited
Loading

  • Overlays: subject.in_gpa, subject.total_socre
  • Refactor round_score
  • Shows subject extra credit

Should close #88

@hey2022 hey2022 requested review from aulimaru and wusitee January 12, 2025 11:05
@hey2022
Copy link
Owner Author

hey2022 commented Jan 12, 2025 •
edited
Loading

@TinghanLi does this fix your edge cases?

@hey2022 hey2022 marked this pull request as ready for review January 12, 2025 11:11
@TinghanLi
Copy link

@TinghanLi does this fix your edge cases?

No. But don't worry, I will find a new one when it's fixed👍

@hey2022 hey2022 marked this pull request as draft January 12, 2025 11:23
@hey2022
Copy link
Owner Author

hey2022 commented Jan 12, 2025

@TinghanLi is beyond help, should work for other people tho.

@hey2022 hey2022 marked this pull request as ready for review January 12, 2025 14:34
wusitee
wusitee reviewed Jan 12, 2025
View reviewed changes
Copy link
Collaborator

@wusitee wusitee left a comment

Choose a reason for hiding this comment

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

Can we add a indication for extra scores?

@hey2022 hey2022 force-pushed the feat/overlay-subject branch from 86dbb9e to 774730d Compare January 12, 2025 15:05
@hey2022 hey2022 requested a review from wusitee January 12, 2025 15:05
@hey2022
Copy link
Owner Author

hey2022 commented Jan 12, 2025

Looks something like:
image

@hey2022 hey2022 force-pushed the feat/overlay-subject branch from 16ef467 to 98b99dd Compare January 12, 2025 17:55
@hey2022
Copy link
Owner Author

hey2022 commented Jan 12, 2025

Rounded extra credit to 1 decimal place, since overlayed score is only precise to 1 decimal place.
image

@wusitee
Copy link
Collaborator

wusitee commented Jan 13, 2025

Rounded extra credit to 1 decimal place, since overlayed score is only precise to 1 decimal place. image

It seems misleading since it is hard to say whether the grade is 97 or 97+0.6.

@wusitee
Copy link
Collaborator

wusitee commented Jan 13, 2025

Putting the extra credit into the course name section is better since there is enough space to make it clearer like AP Economics (with 0.4 extra credit).
image

@hey2022 hey2022 force-pushed the feat/overlay-subject branch from 98b99dd to 6616705 Compare January 13, 2025 05:25
@hey2022
Copy link
Owner Author

hey2022 commented Jan 13, 2025

image

@hey2022 hey2022 force-pushed the feat/overlay-subject branch from 6616705 to 54b20a5 Compare January 13, 2025 20:02
@hey2022
Copy link
Owner Author

hey2022 commented Jan 13, 2025

Is accounting for isInGrade necessary? All middle school subjects are not counted, if we account for this flag, then all calculations will be wrong. Also in @TinghanLi case #92 , isInGrade does not seems to fix the edge cases.

@hey2022 hey2022 force-pushed the feat/overlay-subject branch 3 times, most recently from fceb04b to 17100d5 Compare January 13, 2025 20:35
@hey2022
feat: overlay subject from dynamic score
4a4debf 
- Overlays: subject.in_gpa, subject.total_socre
- Modifies gpa calculation to account for in_gpa flag
- Displays whether a subject is in gpa
@hey2022
feat(subject): store and display extra credit
233868f
@hey2022
refactor: use round_score function to round
364ba4d
@hey2022 hey2022 force-pushed the feat/overlay-subject branch from 17100d5 to 364ba4d Compare January 13, 2025 20:37
@wusitee
Copy link
Collaborator

wusitee commented Jan 14, 2025 •
edited
Loading

After xb is fixed, it can probably be merged.

@hey2022
Copy link
Owner Author

hey2022 commented Jan 20, 2025

It's fixed now

wusitee
wusitee approved these changes Jan 20, 2025
View reviewed changes
Copy link
Collaborator

@wusitee wusitee left a comment

Choose a reason for hiding this comment

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

LGTM

@hey2022 hey2022 merged commit dc59e53 into main Jan 20, 2025
6 checks passed
@hey2022 hey2022 deleted the feat/overlay-subject branch January 20, 2025 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wusitee wusitee wusitee approved these changes

@aulimaru aulimaru Awaiting requested review from aulimaru

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Account for extra credit
3 participants
@hey2022 @TinghanLi @wusitee