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(cost-model): migrate adv-stats #33

Merged
merged 1 commit into from
Nov 15, 2024
Merged

feat(cost-model): migrate adv-stats #33

merged 1 commit into from
Nov 15, 2024

Conversation

xx01cyx
Copy link
Member

@xx01cyx xx01cyx commented Nov 15, 2024

In optd, we use MostCommonValues and Distribution as trait bounds. Because of the serialization & deserialization need in optd-persistent, we use concrete types (enum) here instead of traits.

@xx01cyx xx01cyx changed the title feat(cost-model): migrate stats feat(cost-model): migrate adv-stats Nov 15, 2024
@xx01cyx xx01cyx requested review from lanlou1554 and skyzh November 15, 2024 17:00
Copy link
Member

@skyzh skyzh left a comment

Choose a reason for hiding this comment

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

I would rubber-stamp all patches in this repo to ensure we move fast but I'd like to note that

  • Would be better to have an initial commit to copy-paste everything from the optd repo, and another commit which contains the changes to the original crate. Otherwise I don't know what changes I'll need to review :(
  • Would be better if we can bring all commit history to this repo.

Looking at the dependencies, I saw lazy_static being used, which is already a deprecated crate and replaced by std libraries once cell. Probably needs to be fixed?

Regarding enum -- I'm not sure if this is a good approach regarding extensibility but let's merge it as-is.

@xx01cyx
Copy link
Member Author

xx01cyx commented Nov 15, 2024

Would be better to have an initial commit to copy-paste everything from the optd repo, and another commit which contains the changes to the original crate. Otherwise I don't know what changes I'll need to review :(

Sure. This is actually separated from another larger PR. I just copy-pasted all code... I'll do so next time.

Would be better if we can bring all commit history to this repo.

Is there a good way to do so?

Looking at the dependencies, I saw lazy_static being used, which is already a deprecated crate and replaced by std libraries once cell. Probably needs to be fixed?

I've added a TODO in the code.

Regarding enum -- I'm not sure if this is a good approach regarding extensibility but let's merge it as-is.

I've tried to make MostCommonValues and Distribution as traits, but doesn't work out very well. Not sure if there's a better solution.

@xx01cyx xx01cyx merged commit 91deb07 into main Nov 15, 2024
11 checks passed
@skyzh
Copy link
Member

skyzh commented Nov 15, 2024

https://git-scm.com/docs/git-filter-branch + git merge from an orphan branch

@xx01cyx xx01cyx deleted the cost-model-stats branch November 23, 2024 23:19
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.

2 participants