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

Feat/use median in calculating recall cost, forget cost and learn cost #109

Merged

Conversation

L-M-Sherlock
Copy link
Member

No description provided.

@L-M-Sherlock L-M-Sherlock added the enhancement New feature or request label Apr 25, 2024
@L-M-Sherlock L-M-Sherlock linked an issue Apr 25, 2024 that may be closed by this pull request
@Expertium
Copy link
Contributor

Are zeros removed? As user1823 said, we should remove all values that are precisely equal to 0.

@L-M-Sherlock
Copy link
Member Author

Are zeros removed? As user1823 said, we should remove all values that are precisely equal to 0.

Should these revlogs be removed in training?

@Expertium
Copy link
Contributor

No, I meant that if review time=0 ms, it should be excluded from the median calculation. Though it might be a good idea to exclude them from everything.
@user1823 thoughts?

@user1823
Copy link
Contributor

Though it might be a good idea to exclude them from everything.

I don't think that we should filter them during training the parameters unless someone gives us a specific scenario where they cause a problem.

@L-M-Sherlock
Copy link
Member Author

Now zero values have been removed in calculating the cost.

@user1823
Copy link
Contributor

I can't verify the accuracy of the code but broadly it looks good to me.

@user1823
Copy link
Contributor

Wait a minute, I couldn't find anything that removes the reviews with time > 20 min.

@Expertium
Copy link
Contributor

Expertium commented Apr 25, 2024

I couldn't find anything that removes the reviews with time > 20 min.

Yeah, those should be excluded.

@L-M-Sherlock L-M-Sherlock merged commit cabfe0e into main Apr 25, 2024
1 check passed
@L-M-Sherlock L-M-Sherlock deleted the use-median-in-calculating-recall-cost-and-learn-cost branch April 25, 2024 14:13
@L-M-Sherlock
Copy link
Member Author

Thanks for reviews!

@L-M-Sherlock L-M-Sherlock changed the title Feat/sse median in calculating recall cost, forget cost and learn cost Feat/use median in calculating recall cost, forget cost and learn cost Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use the median instead of the mean for recall costs and learn cost
3 participants