Skip to content

Implement the Sum trait for all AffineCurve - #289

Merged
weikengchen merged 9 commits into
masterfrom
affine-curve-impl-sum
Jun 3, 2021
Merged

Implement the Sum trait for all AffineCurve#289
weikengchen merged 9 commits into
masterfrom
affine-curve-impl-sum

Conversation

@weikengchen

@weikengchen weikengchen commented Jun 1, 2021

Copy link
Copy Markdown
Member

Description

George Gkitsas mentions that it would be convenient to have a Sum trait for AffineCurve, specifically SW curve's GroupAffine.

This PR adds the requirement that all AffineCurve must implement the Sum trait.

It is useful to have this trait: a user who uses + or += in SW's AffineCurve indeed would pay performance overhead --- there are unnecessary conversions between ProjectiveCurve and AffineCurve during each addition. A Sum trait implementation that avoids such unnecessary conversions can significantly boost the performance.

closes: #288


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (master)
  • Linked to Github issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Added a relevant changelog entry to the Pending section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

Feels skippable:

  • Wrote unit tests
  • Updated relevant documentation in the code

@ValarDragon ValarDragon left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

utACK

@weikengchen
weikengchen merged commit 7951133 into master Jun 3, 2021
@weikengchen
weikengchen deleted the affine-curve-impl-sum branch June 3, 2021 03:55
Comment on lines +291 to +292
iter.map(|x| x.into_projective())
.sum::<GroupProjective<P>>()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It would be faster to use mixed addition, no?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good point. Let me double-check. Yes, it is very likely that the conversions from affine to projective may be unnecessary as well.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

(though the latter is cheap)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done in #291

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.

Implement Sum trait for Group elements

3 participants