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

Calculating and Storing Requirement Statistics #866

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

PabloRaigoza
Copy link
Contributor

Summary

This pull request calculates and stores the data of most common courses to fulfil a given requirement and its slots. It only stores the top 50 most popular courses because storage limitations on Firestore. In addition, this pull request is going to be become important for course recommendations based on most popular courses to fulfill a requirement.

I also cleaned up the code for 'scripts/firebase-config.ts' to be a little more consistant as well as add a new npm command to run most if not all of the cli checks.

  • Calculating Requirement Statistics
  • Soring Requirement Statistics

Testing

I tested this script on the dev server by adding a courses on my account, and the changes did appear in Firestore. There are some users that error when attempting to calculate their requirement statistics, by this is likely due to corrupt data in the Firestore.

@PabloRaigoza PabloRaigoza requested a review from a team as a code owner October 23, 2023 01:00
@dti-github-bot
Copy link
Member

dti-github-bot commented Oct 23, 2023

[diff-counting] Significant lines: 187.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 23, 2023

Visit the preview URL for this PR (updated for commit 95acbc1):

https://cornelldti-courseplan-dev--pr866-pablo-fulfillment-st-fgfvlq8w.web.app

(expires Thu, 28 Dec 2023 18:50:37 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 6d4e0437c4559ed895272bbd63991394f1e0e933

Copy link
Collaborator

@elizabeth-tang elizabeth-tang left a comment

Choose a reason for hiding this comment

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

This looks really good Pablo! I just had a few small suggestions about code cleaning and console statements. Excited to get started on "recommendations"!!

scripts/gen-req-full-stats.ts Outdated Show resolved Hide resolved
scripts/gen-req-full-stats.ts Outdated Show resolved Hide resolved
scripts/gen-req-full-stats.ts Outdated Show resolved Hide resolved
scripts/gen-req-full-stats.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@zachary-kent zachary-kent left a comment

Choose a reason for hiding this comment

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

Just a couple minor nits!

scripts/gen-req-full-stats.ts Outdated Show resolved Hide resolved
src/requirements/fulfillment-stats.ts Outdated Show resolved Hide resolved
scripts/gen-req-full-stats.ts Outdated Show resolved Hide resolved
scripts/gen-req-full-stats.ts Outdated Show resolved Hide resolved
src/requirements/fulfillment-stats.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@zachary-kent zachary-kent left a comment

Choose a reason for hiding this comment

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

Looks mostly good, just a few small comments?

);
} catch {
// There was an error computing the fulfillment stats for the user
console.log(`${numberOfErrors} : Error computing fulfillment stats for ${doc.id}`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
console.log(`${numberOfErrors} : Error computing fulfillment stats for ${doc.id}`);
console.error(`${numberOfErrors} : Error computing fulfillment stats for ${doc.id}`);

Comment on lines 34 to 65
await semQuerySnapshot.forEach(async doc => {
// obtain the user's semesters, onboarding data, etc...
const semesters = (await doc.data()).semesters ?? {};
const onboardingData = (await onboardingDataCollection.doc(doc.id).get()).data() ?? {};
const toggleableRequirementChoices =
(await toggleableRequirementChoicesCollection.doc(doc.id).get()).data() ?? {};
const overriddenFulfillmentChoices =
(await overriddenFulfillmentChoicesCollection.doc(doc.id).get()).data() ?? {};

// Attempt to compute the fulfillment stats for the user
try {
// use createAppOnboardingData to convert the onboarding data to the format used by the frontend
const newOnboardingData = await createAppOnboardingData(onboardingData);

// compute the fulfillment stats
const res = await computeGroupedRequirementFulfillmentReports(
semesters,
newOnboardingData,
toggleableRequirementChoices,
overriddenFulfillmentChoices
);

await computeFulfillmentStats(
res.groupedRequirementFulfillmentReport,
idRequirementFrequency
);
} catch {
// There was an error computing the fulfillment stats for the user
console.log(`${numberOfErrors} : Error computing fulfillment stats for ${doc.id}`);
numberOfErrors += 1;
}
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
await semQuerySnapshot.forEach(async doc => {
// obtain the user's semesters, onboarding data, etc...
const semesters = (await doc.data()).semesters ?? {};
const onboardingData = (await onboardingDataCollection.doc(doc.id).get()).data() ?? {};
const toggleableRequirementChoices =
(await toggleableRequirementChoicesCollection.doc(doc.id).get()).data() ?? {};
const overriddenFulfillmentChoices =
(await overriddenFulfillmentChoicesCollection.doc(doc.id).get()).data() ?? {};
// Attempt to compute the fulfillment stats for the user
try {
// use createAppOnboardingData to convert the onboarding data to the format used by the frontend
const newOnboardingData = await createAppOnboardingData(onboardingData);
// compute the fulfillment stats
const res = await computeGroupedRequirementFulfillmentReports(
semesters,
newOnboardingData,
toggleableRequirementChoices,
overriddenFulfillmentChoices
);
await computeFulfillmentStats(
res.groupedRequirementFulfillmentReport,
idRequirementFrequency
);
} catch {
// There was an error computing the fulfillment stats for the user
console.log(`${numberOfErrors} : Error computing fulfillment stats for ${doc.id}`);
numberOfErrors += 1;
}
});
await Promise.all(semQuerySnapshot.map(async doc => {
// obtain the user's semesters, onboarding data, etc...
const semesters = (await doc.data()).semesters ?? {};
const onboardingData = (await onboardingDataCollection.doc(doc.id).get()).data() ?? {};
const toggleableRequirementChoices =
(await toggleableRequirementChoicesCollection.doc(doc.id).get()).data() ?? {};
const overriddenFulfillmentChoices =
(await overriddenFulfillmentChoicesCollection.doc(doc.id).get()).data() ?? {};
// Attempt to compute the fulfillment stats for the user
try {
// use createAppOnboardingData to convert the onboarding data to the format used by the frontend
const newOnboardingData = await createAppOnboardingData(onboardingData);
// compute the fulfillment stats
const res = await computeGroupedRequirementFulfillmentReports(
semesters,
newOnboardingData,
toggleableRequirementChoices,
overriddenFulfillmentChoices
);
await computeFulfillmentStats(
res.groupedRequirementFulfillmentReport,
idRequirementFrequency
);
} catch {
// There was an error computing the fulfillment stats for the user
console.log(`${numberOfErrors} : Error computing fulfillment stats for ${doc.id}`);
numberOfErrors += 1;
}
}));

const newSlot = new Map<number, number>();
const sorted = [...slot.entries()].sort((a, b) => b[1] - a[1]);

const numberOfCourses = sorted.length > 50 ? 50 : sorted.length;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const numberOfCourses = sorted.length > 50 ? 50 : sorted.length;
const numberOfCourses = Math.min(sorted.length, 50);

* @param _callback is a function that is called after the fulfillment stats have been computed
* @throws Error when computeGroupedRequirementFulfillmentReports fails to compute the fulfillment stats
*/
async function computeRequirementFullfillmentStatistics(_callback) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need this _callback argument?

Comment on lines +42 to +46
for (let j = 0; j < currentCourseSlot.length; j += 1) {
const currentCourseId = currentCourseSlot[j].courseId;
const pastFreq = currentRequirementSlotFreq.get(currentCourseId) ?? 0;
currentRequirementSlotFreq.set(currentCourseId, pastFreq + 1);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you convert this into a for each loop?

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