Update exam clash detection in the Module search page

uyencfi commented 3 months ago

Context

Fix #3663

Implementation

Given timetable = [m1, m2, m3] which is the list of modules currently added in the timetable. To detect whether a module X clashes with timetable:

current behaviour is to compare X.start == m.start (exact match) for each m in timetable
updated behaviour:
- assert that (X.start < m.end) && (X.end > m.start) for each m in timetable. This is the condition for 2 intervals to overlap, see https://stackoverflow.com/questions/325933/determine-whether-two-date-ranges-overlap

❗ However, the module struct only stores examStartTime (ISO timestamp, assuming UTC(?)) and examDuration (in minutes). Thus to get the endTime:

m.end can be easily obtained since m is a known value from the timetable.
X.end is more difficult since we need to compute it on-the-fly inside the Elasticsearch query.

The new Elasticsearch query I have looks like

query: {
  bool: { must_not: [  // this document must not match *any* condition in the list, i.e. not overlap any exam

    bool: { must: {               // condition 1: true if this exam overlaps m1
      range: { document.start < m1.start + m1.duration },
      script: { document.start + document.duration > m1.start }   // <-- can't use 'range' b/c ES doesn't allow accessing 2 fields at the same time
    }},

    bool: { must: { ... } },     // condition 2: true if this exam overlaps m2
    bool: { must: { ... } },     // condition 3: true if this exam overlaps m3
  ]},
},

TBD

Is examStartTime really stored in Elasticsearch in UTC or UTC+08?
(Unit?) Testing and how to verify correctness

vercel[bot] commented 3 months ago

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	Apr 3, 2024 9:12am
nusmods-website	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Apr 3, 2024 9:12am

vercel[bot] commented 3 months ago

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

@nusmodifications first needs to authorize it.

kokrui commented 3 months ago

Thanks for the PR! We'll test out scraper changes over the next few days and get back to you ☺️

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 0% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 53.50%. Comparing base (19f1480) to head (7ca18ec). Report is 1 commits behind head on master.

Files	Patch %	Lines
website/src/views/modules/ModuleFinderSidebar.tsx	0.00%	12 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #3678 +/- ## ========================================== - Coverage 53.57% 53.50% -0.08% ========================================== Files 273 273 Lines 5984 5992 +8 Branches 1430 1431 +1 ========================================== Hits 3206 3206 - Misses 2778 2786 +8 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

kokrui commented 3 months ago

Quick live test case:

Add CM2112 to your Semester 2 timetable
In course search, enable "No Exam Clash (Sem 2)"
Search for HSI2001
You should not see HSI2001

kokrui commented 2 months ago

Hey, it looks like the exam for CM2112 got changed to be 2.30pm instead of 1pm, which makes it match HSI2001, so it's no longer that good of a candidate for a test case. A valid test case still exists: Sem 1, SH5110 vs FE5222

That said, let me take a look and test some things with your query and get back to you -- it looks pretty good right now, but for one, I think the query is inverted! (i.e. "No Exam Clash" gives us courses which do have an exam clash)

nusmodifications / nusmods