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

Update exam clash detection in the Module search page #3678

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

uyencfi
Copy link

@uyencfi uyencfi commented Mar 20, 2024

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:

❗ 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

Copy link

vercel bot commented Mar 20, 2024

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

Copy link

vercel bot commented Mar 20, 2024

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

@nusmodifications first needs to authorize it.

@kokrui
Copy link
Member

kokrui commented Mar 20, 2024

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

Copy link

codecov bot commented Mar 20, 2024

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 ⚠️
Additional details and impacted files
@@            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     

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

@kokrui
Copy link
Member

kokrui commented Apr 3, 2024

Quick live test case:

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

@kokrui
Copy link
Member

kokrui commented Apr 11, 2024

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)

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.

Improve module search page's exam clash detection
2 participants