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

Minor fixes #85

Merged
merged 6 commits into from
Sep 29, 2024
Merged

Minor fixes #85

merged 6 commits into from
Sep 29, 2024

Conversation

dipamsen
Copy link
Contributor

  • Fixed 'X' lab slot having extra hour (according to latest Central Time Table)
  • check for argv[1]'s existence before access

On running the script there seems to be a huge diff on data/data.json, which seems to be largely due to order changes (along with few actual changes to the data as well). Is the order of slots, rooms random for some reason?

@proffapt
Copy link
Member

Hi, @dipamsen thanks for the contribution!!

We were indeed reported some inconsistencies in the displayed time-table for some professors. I am attaching those incosistencies in this message so that you can verify that your fix actually fixes them as well. If this doesn't you may try to solve it (in this same or different PR).

image
image

Look for EC60101.

cc: @harshkhandeparkar @Faizan2005 @Prasanthi-Peram @rohan-b-84

@dipamsen
Copy link
Contributor Author

dipamsen commented Sep 28, 2024

@proffapt Thanks!

I found out the problem in the above example is that when multiple courses occupy the same slot [1], only the first one found is shown in the frontend. I have changed it to allow more than one course to be present in one slot. (I have not modified any styles or structure of the data, just changed the function getTimeSlotInfo to return an array of courses rather than a single one.)

image

[1]: This can happen because there are courses which are divided into sections, and one professor only teaches one section. However the ERP does not give any details about which professor teaches which section, so we are forced to show all possible slots where the class could be happening, even if the professor may not be present there (like seen above for EC29201)

@proffapt
Copy link
Member

proffapt commented Sep 28, 2024

Great work!

Can you verify this prof's details too? or share the timetable after your fixes?

image
EC60013

Once we get this verified and get approval for your code changes, then this PR will be merged.

[1]: This can happen because there are courses which are divided into sections, and one professor only teaches one section. However the ERP does not give any details about which professor teaches which section, so we are forced to show all possible slots where the class could be happening, even if the professor may not be present there (like seen above for EC29201)

Makes sense. I guess we can add a disclaimer or a collapsable tooltip which tells the user about this nuances.
cc: @Prasanthi-Peram @Faizan2005 @harshkhandeparkar @rohan-b-84

@dipamsen
Copy link
Contributor Author

@proffapt Sure thing!

image

@@ -102,7 +102,7 @@
"V2": [(3, 6), (3, 7)],
"V3": [(3, 6), (3, 7), (4, 6)],
"V4": [(3, 6), (3, 7), (4, 5), (4, 6)],
"X": [(2, 5), (2, 6), (2, 7), (2, 8)],
"X": [(2, 5), (2, 6), (2, 7)],
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't the X slot have 4 hours here?

Copy link
Member

Choose a reason for hiding this comment

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

Technically it has 4 hours but only 3 of those are given to the lab slot X. Are there any examples where the 4th hour is also used? And if yes, how is it mentioned?

Copy link
Contributor Author

@dipamsen dipamsen Sep 28, 2024

Choose a reason for hiding this comment

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

No, because as I understand it, just "X" refers to the Lab Slot X (at least from our data source, i.e. "Subject List with Slots" in the ERP), which is just 3 hours long (as are all lab slots). "X4" would refer to the four hour slot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the fourth slot is used, it would be mentioned as "X44" (fourth slot of X4)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Example of fourth hour of X4 being used

image

Copy link
Member

Choose a reason for hiding this comment

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

We should handle that case as well. Any ideas on how that could be done?

Copy link

@rohan-b-84 rohan-b-84 Sep 28, 2024

Choose a reason for hiding this comment

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

just "X" refers to the Lab Slot X (at least from our data source, i.e. "Subject List with Slots" in the ERP), which is just 3 hours long (as are all lab slots). "X4" would refer to the four hour slot

afaik, this is the case.
If only X is mentioned, then its the Lab Slot X
If its X with numbers, then its the courses, which can also (or only) have 4th 5-6pm slot.

Choose a reason for hiding this comment

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

Any ideas on how that could be done?

I guess, we can "X" mark the first 3 hours, and "X4n" should mark the corresponding 1hour slots ??

Copy link
Contributor Author

@dipamsen dipamsen Sep 28, 2024

Choose a reason for hiding this comment

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

@rohan-b-84 I think this is already handled? I havent looked at the code, but it seems to work?
eg: (X4 slots - wednesday 2pm to 6pm)

image

Without having looked at the code, I believe Aij is handled as Ai[j-1], because Ai always is a list of slots.

Also, this notation is not specific to X slot, eg. S33 refers to third hour of S3 slot.

Copy link
Member

@harshkhandeparkar harshkhandeparkar left a comment

Choose a reason for hiding this comment

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

Have you committed the Python changes?

@harshkhandeparkar
Copy link
Member

On running the script there seems to be a huge diff on data/data.json, which seems to be largely due to order changes (along with few actual changes to the data as well). Is the order of slots, rooms random for some reason?

I think it is because the timetables are fetched in random order.

@dipamsen
Copy link
Contributor Author

Have you committed the Python changes?

Yes, there are only two lines of change :) (the multiple courses per slot thing was purely a frontend change)

@proffapt
Copy link
Member

@proffapt Sure thing!

image

The one who reported this inconsistency has verified the correctness of this new time table.
Logic seems working fine. We can now proceed to actual code review.

cc: @harshkhandeparkar @rohan-b-84

@proffapt
Copy link
Member

Have you committed the Python changes?

Yes, there are only two lines of change :) (the multiple courses per slot thing was purely a frontend change)

Not exactly. The frontend displays the timetable structure taking data from data.json and incosistencies.json. Your changes fixed issues in these files and that's what the frontend displayed. So it's fine.

@dipamsen
Copy link
Contributor Author

Not exactly. The frontend displays the timetable structure taking data from data.json and incosistencies.json. Your changes fixed issues in these files and that's what the frontend displayed. So it's fine.

Well, the data.json did already include information about the multiple courses in a single slot. It lists each course for a given professor, and for each course what slots it is present in. So naturally if two courses are present in one slot, that information is already available in data.json.

The only bug was in the frontend, wherein for each slot, the code searched for the first course in the array that occupied that slot. (It failed to account for the fact that there can be multiple such courses that occupies this slot)

@proffapt
Copy link
Member

Not exactly. The frontend displays the timetable structure taking data from data.json and incosistencies.json. Your changes fixed issues in these files and that's what the frontend displayed. So it's fine.

Well, the data.json did already include information about the multiple courses in a single slot. It lists each course for a given professor, and for each course what slots it is present in. So naturally if two courses are present in one slot, that information is already available in data.json.

The only bug was in the frontend, wherein for each slot, the code searched for the first course in the array that occupied that slot. (It failed to account for the fact that there can be multiple such courses that occupies this slot)

Ok, makes sense.

Copy link
Member

@harshkhandeparkar harshkhandeparkar left a comment

Choose a reason for hiding this comment

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

LGTM. Waiting for @rohan-b-84's review.

@rohan-b-84
Copy link

LGTM.

I would say we should add some consistent formatting/linting, and make the output more deterministic... but that can be another issue

@proffapt
Copy link
Member

Fine then, merging the PR and can discuss the linting part in a new issue.

@proffapt proffapt merged commit b6cea5a into metakgp:master Sep 29, 2024
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.

4 participants