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

Fix leading coefficients might be zero #800

Merged
merged 1 commit into from
Dec 19, 2023

Conversation

moCello
Copy link
Member

@moCello moCello commented Dec 14, 2023

Resolves #796

@moCello moCello force-pushed the mocello/796_leading_coefficient branch 2 times, most recently from 66a28b3 to 375a861 Compare December 14, 2023 16:08
Copy link

codecov bot commented Dec 14, 2023

Codecov Report

Merging #800 (4e21aae) into master (902396e) will increase coverage by 0.55%.
The diff coverage is 88.31%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #800      +/-   ##
==========================================
+ Coverage   85.10%   85.65%   +0.55%     
==========================================
  Files          57       57              
  Lines        4047     4119      +72     
==========================================
+ Hits         3444     3528      +84     
+ Misses        603      591      -12     
Files Coverage Δ
src/fft/polynomial.rs 69.03% <88.31%> (+22.21%) ⬆️

... and 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 902396e...4e21aae. Read the comment docs.

@moCello moCello force-pushed the mocello/796_leading_coefficient branch 6 times, most recently from 3f2462e to 24e2fa0 Compare December 18, 2023 14:42
Copy link
Member

@HDauven HDauven 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
Member

@xevisalle xevisalle left a comment

Choose a reason for hiding this comment

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

LGTM, just some nits

@@ -113,30 +118,35 @@ impl Polynomial {
}
}

/// Evaluates a [`Polynomial`] at a given point in the field.
pub(crate) fn evaluate(&self, point: &BlsScalar) -> BlsScalar {
/// Evaluates a [`Polynomial`] at a given scalar in the field.
Copy link
Member

Choose a reason for hiding this comment

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

even when is the common way to say it, indeed was a bit confusing to talk about 'points' in this context, as it could seem that it refers to a point in the curve. However, I would not put 'scalar' here as it is not a scalar (even when the type is). I would say 'value'.

Copy link
Member Author

Choose a reason for hiding this comment

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

you are right! I changed it

@@ -91,16 +90,22 @@ impl Polynomial {
result
}

/// Returns the degree of the [`Polynomial`].
/// Returns the degree, i.e. the index of the highest non-zero coefficient,
Copy link
Member

Choose a reason for hiding this comment

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

I understand what you say, but could it seem that you are taking the highest coefficient? Maybe say the highest index among all coeffs...?

src/fft/polynomial.rs Show resolved Hide resolved
@moCello moCello force-pushed the mocello/796_leading_coefficient branch from 9f63f69 to 4e21aae Compare December 19, 2023 13:02
@moCello moCello requested a review from xevisalle December 19, 2023 13:02
@moCello moCello merged commit 1f655c2 into master Dec 19, 2023
10 checks passed
@moCello moCello deleted the mocello/796_leading_coefficient branch December 19, 2023 13:24
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.

leading_coefficient() may unexpectedly be zero
3 participants