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

Add in_place suffix for BigInteger methods #782

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tcoratger
Copy link
Contributor

@tcoratger tcoratger commented Feb 16, 2024

Description

This pull request introduces a standardized naming convention for methods in the BigInteger module, specifically targeting functions where the self mutable value is modified in-place. The convention adds the in_place suffix to method names to provide clarity and consistency in the codebase.

closes: #781


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

@tcoratger tcoratger requested review from a team as code owners February 16, 2024 17:39
@tcoratger tcoratger requested review from z-tech, mmagician and weikengchen and removed request for a team February 16, 2024 17:39
@tcoratger
Copy link
Contributor Author

@Pratyush I thought about this PR and because the MulAssign or AddAssign traits of the Rust library use the suffix Assign, we will necessarily have mul_assign or add_assign which will be present somewhere in the code.

To unify everything, wouldn't it be better to rename everything throughout the code with an assign suffix rather than in_place. And for example the instances of square_in_place would also become square_assign, what do you think?

@mmaker
Copy link
Member

mmaker commented Feb 28, 2024

I'd much rather have a _assign indeed. What do people think?

@Pratyush
Copy link
Member

Wouldn’t it be a massive breaking change?

@mmaker
Copy link
Member

mmaker commented Feb 28, 2024

Not more than adding thein_place suffix? If it helps with naming consistency down the line before 1.0 I think that's a valid effort (and we should deprecate the previous functions, not just remove them)

@Pratyush
Copy link
Member

Pratyush commented Feb 28, 2024 via email

@mmaker
Copy link
Member

mmaker commented Feb 28, 2024

[EDIT]To phrase better my comment:

In Field we have

double_in_place (from AdditiveGroup)
neg_in_place (from AdditiveGroup)
sqrt_in_place
square_in_place
inverse_in_place
frobenius_map_in_place

I think it's nice to have either (1) in_place everywhere except trait implementations of std::ops::*, OR (2) _assign everywhere. I'd be consistent with whatever we pick, and propagate this change to ark-ff and ark-ec, deprecating old function calls that will be removed for 1.0.
I fear having intermediate solutions will make the naming conventions of the library confusing.

@Pratyush In your comment, are you implying that having _assign only for BigInt is acceptable?
@tcoratger What do you think?

@tcoratger
Copy link
Contributor Author

@mmaker Agreed, I found it confusing at the beginning like in Montgomery backend for exemple we have:

  • add_assign
  • sub_assign
  • double_in_place
  • neg_in_place

The std library always uses the _assign suffix for all operations so for me it would be worth changing everything to have consistent naming everywhere (_assign in a std like style) in the codebase. But would agree that this is quite a massive change of a big part of the function names of the whole library.

@burdges
Copy link
Contributor

burdges commented Feb 29, 2024

"Assignment operators" is the C term, so _assign made sense for operators, and hence maybe sensible for methods resembling operator methods, ala mul2_assign.

Afaik _in_place was always more traditional overall, and std uses in_place for pointers, so nothing wrong with _in_place either, but maybe resembling operators should be the priority.

Also, big integers are bigish for high performace code, so if a non-in-place variant feels particularly rediculous for operation foo, then foo could be the in-place variant, and _cloned could be the non-in-place variant, but again maybe resembling operators should be the priority.

@Pratyush
Copy link
Member

So my rough rule-of-thumb has been to use _assign for the binary operators, and _in_place for the unary ones (negate_in_place, square_in_place, etc). Happy to uniformize the notation to avoid confusion though.

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.

Use in_place suffix for BigInteger operations
4 participants