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

Add additional check for findExamClashes() for non-identical start times #3560

Merged
merged 13 commits into from
Feb 19, 2024

Conversation

EssWhyy
Copy link
Contributor

@EssWhyy EssWhyy commented Aug 12, 2023

Context

Fix issue #3449

Implementation

Per the discussion in the issue, I added in an extra check in findExamClashes() in timetables.ts. The steps for this check are as follows:

  1. Group modules in timetable by exam date only
  2. For each array of modules[] assigned to an exam date, we do a brute force O(n^2) check on each unique pair of modules
  3. If two modules have the same exam start date and time, then we ignore since its already accounted for in the existing clash check
  4. If two modules with different exam start time has clash based on time intervals, then we add it into the existing clashes dictionary via the dateTime string key

The result can be seen here for both test cases:

case1
case2

Other Information

The current exam clash warning text can only show one date & time, as such for two exams with different start times, I used the earlier start time for the warning text as a workaround for now.

The function could probably be less verbose and more efficient. But on testing my end, there are no major slowdowns on the performance when adding or deleting a typical number of modules to the timetable.

@vercel
Copy link

vercel bot commented Aug 12, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nusmods-export ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 19, 2024 3:56pm
nusmods-website ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 19, 2024 3:56pm

@vercel
Copy link

vercel bot commented Aug 12, 2023

@EssWhyy is attempting to deploy a commit to a Personal Account owned by @nusmodifications on Vercel.

@nusmodifications first needs to authorize it.

Copy link

codecov bot commented Feb 19, 2024

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (84f4c5a) 53.20% compared to head (a48a423) 53.48%.

Files Patch % Lines
website/src/utils/timetables.ts 92.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3560      +/-   ##
==========================================
+ Coverage   53.20%   53.48%   +0.27%     
==========================================
  Files         272      272              
  Lines        5885     5931      +46     
  Branches     1397     1408      +11     
==========================================
+ Hits         3131     3172      +41     
- Misses       2754     2759       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@kokrui kokrui left a comment

Choose a reason for hiding this comment

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

Thanks @EssWhyy for getting this started! It's an important change and it's ready to land now with some algo changes + tests

@kokrui kokrui merged commit 82927f6 into nusmodifications:master Feb 19, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants