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(math): impl remove zeros for sparse univariate poly #430

Merged

Conversation

batzor
Copy link
Contributor

@batzor batzor commented Jun 7, 2024

Description

  • Change RemoveHighDegreeZeros() to RemoveZeros() in univariate spare polynomial
  • Enforce clean construction of polynomials. After this change, all poly operations are expected to operate on clean polynomials and output clean polynomials.

@batzor batzor force-pushed the feat(math)/remove-zeros-for-sparse-poly branch from 9a9b1af to 155f934 Compare June 7, 2024 04:09
@batzor batzor closed this Jun 7, 2024
@batzor batzor force-pushed the feat(math)/remove-zeros-for-sparse-poly branch from 155f934 to 78fd8bd Compare June 7, 2024 04:14
@batzor batzor reopened this Jun 7, 2024
@batzor batzor force-pushed the feat(math)/remove-zeros-for-sparse-poly branch from 29ebb3c to 23f5160 Compare June 7, 2024 05:35
@batzor batzor force-pushed the feat(math)/remove-zeros-for-sparse-poly branch from 23f5160 to 742336a Compare June 7, 2024 07:00
Copy link
Contributor

@dongchangYoo dongchangYoo left a comment

Choose a reason for hiding this comment

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

LGTM

@batzor batzor force-pushed the feat(math)/remove-zeros-for-sparse-poly branch 2 times, most recently from ae88fb4 to 9d78494 Compare June 10, 2024 06:31
Copy link
Contributor

@Insun35 Insun35 left a comment

Choose a reason for hiding this comment

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

LGTM

@batzor batzor force-pushed the feat(math)/remove-zeros-for-sparse-poly branch from 9d78494 to 4394f2c Compare June 10, 2024 08:25
Copy link
Contributor

@ashjeong ashjeong left a comment

Choose a reason for hiding this comment

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

LGTM

@TomTaehoonKim
Copy link
Contributor

@batzor For the second commit, I can agree for the sparse coefficients but not with the dense coefficients. What's the motivation behind reducing the usage of remove high degree zeros for dense coefficients?

@batzor
Copy link
Contributor Author

batzor commented Jun 10, 2024

@TomTaehoonKim Indeed, the RemoveHighDegreeZeros() is low cost operation so it's not that necessary but I just did the change for the sake of consistency and clarity.

@batzor batzor force-pushed the feat(math)/remove-zeros-for-sparse-poly branch 2 times, most recently from 250246b to 6561174 Compare June 11, 2024 01:57
@batzor batzor force-pushed the feat(math)/remove-zeros-for-sparse-poly branch from 6561174 to 65131d2 Compare June 11, 2024 06:50
@batzor
Copy link
Contributor Author

batzor commented Jun 11, 2024

@chokobole I reordered the commits as you requested. Hope its more clear now;;

@batzor batzor force-pushed the feat(math)/remove-zeros-for-sparse-poly branch from 65131d2 to 802ef70 Compare June 12, 2024 01:11
Copy link
Contributor

@chokobole chokobole left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@TomTaehoonKim TomTaehoonKim left a comment

Choose a reason for hiding this comment

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

LGTM

@chokobole chokobole merged commit 14a26a2 into kroma-network:main Jun 13, 2024
3 checks passed
Insun35 added a commit that referenced this pull request Jun 13, 2024
Insun35 added a commit that referenced this pull request Jun 13, 2024
Insun35 added a commit that referenced this pull request Jun 13, 2024
Insun35 added a commit that referenced this pull request Jun 13, 2024
chokobole pushed a commit that referenced this pull request Jun 15, 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.

6 participants