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

Improve documentation via better READMEs #508

Merged
merged 9 commits into from
Nov 3, 2022
Merged

Conversation

Pratyush
Copy link
Member

@Pratyush Pratyush commented Nov 2, 2022

Description

Improves the documentation for ark-ff, ark-ec, and ark-serialize by directly incorporating the content of ark-algebra-intro.


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.
  • Wrote unit tests
  • Updated relevant documentation in the code
  • Added a relevant changelog entry to the Pending section in CHANGELOG.md
  • Re-reviewed Files changed in the GitHub PR explorer

@Pratyush Pratyush requested review from a team as code owners November 2, 2022 05:57
@Pratyush Pratyush requested review from mmagician and weikengchen and removed request for a team November 2, 2022 05:57
Copy link
Member

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

Started reviewing, this is quite nice, nice job!

I don't think I'll be able to review much more, but thought I'd leave a comment!


let s1 = ScalarField::rand(&mut rng);
let s2 = ScalarField::rand(&mut rng);
let result = G::msm(&[a, b], &[s1, s2]);
Copy link
Member

Choose a reason for hiding this comment

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

something that reads odd, why is it group MSM, but GAffine base points?

(I understand the reason, just not sure if we want to note anything about this in README, or for simplicity make a,b in G)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. I'll add some explanation there.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added an explanation in the example; let me know if this looks better =)

Copy link
Member

@mmagician mmagician left a comment

Choose a reason for hiding this comment

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

The docs read very well, it's a great improvement!

@Pratyush Pratyush requested a review from mmagician November 3, 2022 15:04
@Pratyush Pratyush merged commit cab3eb9 into master Nov 3, 2022
@Pratyush Pratyush deleted the feat/better-readmes branch November 3, 2022 16:32
@mmagician mmagician mentioned this pull request Dec 22, 2022
4 tasks
andrewmilson added a commit to andrewmilson/algebra that referenced this pull request Jan 1, 2023
* upstream/master:
  Update hashbrown requirement from 0.12.1 to 0.13.1 (arkworks-rs#515)
  Update libtest-mimic requirement from 0.5.2 to 0.6.0 (arkworks-rs#514)
  Update changelog (arkworks-rs#511)
  Add a Discord link (arkworks-rs#510)
  Improve documentation via better READMEs (arkworks-rs#508)
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