Skip to content

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.

3 participants