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 simple methods to convert to and from bytes for ZZ and finite fields #37343

Merged
merged 5 commits into from
Mar 25, 2024

Conversation

GiacomoPope
Copy link
Contributor

@GiacomoPope GiacomoPope commented Feb 14, 2024

I often have to work with the conversion of bytes / integers and currently do this by working with int types and then casting things to ZZ or elements of finite fields.

This PR is a small addition to Integer and FiniteField types which allow this with simply functions which internally convert to / from int for the user so that things can be done naively from SageMath types.

There's a TODO in the code which acknowledges that a faster method for to_bytes might be to work straight from the gmp object in cython for the integers, but it wasn't obvious to me how to do this.

Examples

sage: ZZ.from_bytes(b'\x00\x10', byteorder='big')
16
sage: ZZ.from_bytes(b'\x00\x10', byteorder='little')
4096
sage: ZZ.from_bytes(b'\xfc\x00', byteorder='big', is_signed=True)
-1024
sage: ZZ.from_bytes(b'\xfc\x00', byteorder='big', is_signed=False)
64512
sage: ZZ.from_bytes([255, 0, 0], byteorder='big')
16711680
sage: type(_)
<class 'sage.rings.integer.Integer'>
sage: (1024).to_bytes(2, byteorder='big')
b'\x04\x00'
sage: (1024).to_bytes(10, byteorder='big')
b'\x00\x00\x00\x00\x00\x00\x00\x00\x04\x00'
sage: (-1024).to_bytes(10, byteorder='big', is_signed=True)
b'\xff\xff\xff\xff\xff\xff\xff\xff\xfc\x00'
sage: x = 1000
sage: x.to_bytes((x.bit_length() + 7) // 8, byteorder='little')
b'\xe8\x03'
sage: F = GF(65537)
sage: a = F.random_element()
sage: a.to_bytes()
b'\x00\n\x86'
sage: F.from_bytes(_)
2694
sage: a
2694
sage: F = GF(3^10)
sage: a = F.random_element(); a
z10^8 + z10^7 + 2*z10^5 + 2*z10^4 + 2*z10^3 + z10^2 + z10 + 1
sage: a.to_bytes()
b'$\xf7'
sage: F.from_bytes(_)
z10^8 + z10^7 + 2*z10^5 + 2*z10^4 + 2*z10^3 + z10^2 + z10 + 1

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

@GiacomoPope
Copy link
Contributor Author

GiacomoPope commented Feb 14, 2024

I decided to implement the below, so this comment is redundant

An extension of this PR would be to do the same for finite fields, where the length of the bytes is set by the characteristic automatically... Not sure this needs to be done in the same PR though as i imagine people may have opinions about this

@grhkm21
Copy link
Contributor

grhkm21 commented Feb 15, 2024

Linting fails

@grhkm21
Copy link
Contributor

grhkm21 commented Feb 15, 2024

Looks good to me

@GiacomoPope GiacomoPope changed the title Add simple methods to convert Integers to and from bytes Add simple methods to convert to and from bytes for ZZ and finite fields Feb 15, 2024
@GiacomoPope
Copy link
Contributor Author

Added the review label back in because I added more features to the PR

@GiacomoPope
Copy link
Contributor Author

@grhkm21 @yyyyx4 if either of you have time to look at the extended PR to include FF as well as ZZ

Copy link
Contributor

@grhkm21 grhkm21 left a comment

Choose a reason for hiding this comment

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

Small changes.

src/sage/rings/finite_rings/element_base.pyx Outdated Show resolved Hide resolved
src/sage/rings/finite_rings/element_base.pyx Outdated Show resolved Hide resolved
src/sage/rings/finite_rings/finite_field_base.pyx Outdated Show resolved Hide resolved
src/sage/rings/finite_rings/finite_field_base.pyx Outdated Show resolved Hide resolved
src/sage/rings/integer.pyx Outdated Show resolved Hide resolved
Comment on lines 7187 to 7188
TODO: should we convert straight from the gmp type in cython? This is definitely
possible but I'm not sure the cleanest way to do this
Copy link
Contributor

Choose a reason for hiding this comment

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

I can have a look at that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, wanna look before merging the PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes

@GiacomoPope
Copy link
Contributor Author

Committed changes. Thanks for your review.

@grhkm21
Copy link
Contributor

grhkm21 commented Feb 15, 2024

Looks good to me, but changed to s: needs work because I will work on it.

If I don't finish in a day or two then better merge it first.

@tscrim
Copy link
Collaborator

tscrim commented Feb 20, 2024

Note that one-line descriptions should be one line (=sentence). So the part about the internal bit should go on its own paragraph.

Also please use .. TODO::.

@GiacomoPope
Copy link
Contributor Author

Thanks Travis. I made changes from your feedback. Since it was five days since Gareth suggested working on the optimisation for conversion to GMP I may remove the needs work if that's OK? I dont think it's an important optimisation for the method.

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

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

Thanks. Yes, I think you can remove the needs work. (Either way, that isn't anything that requires this to be held up as it can always be done on a subsequent PR.)

I think the OUTPUT: blocks are gratuitous, but if you want to keep them, either no capital letter for the bullet point or remove the bullet and make it a full sentence with a period/full-stop.

Just as a comment: I also dislike the blanklines between input blocks as I find it more complicated to parse quickly. However, this is up to individual authors, but I feel compelled to mention it from time to time.

src/sage/rings/integer.pyx Outdated Show resolved Hide resolved
src/sage/rings/integer.pyx Outdated Show resolved Hide resolved
src/sage/rings/finite_rings/element_base.pyx Outdated Show resolved Hide resolved
src/sage/rings/integer.pyx Outdated Show resolved Hide resolved
@grhkm21
Copy link
Contributor

grhkm21 commented Feb 20, 2024

Hi sorry, I’m three days behind on all GitHub discussions 🥲 very sorry for the lack of update. I’ll do it on a separate PR

@GiacomoPope
Copy link
Contributor Author

I removed the needs work tab and commited the suggested changes. I added the "needs review" back in simply as I didnt want to label my own PR as "positive review". Will let the CI run and I suppose someone else can add the final label when they have time to double check the latest commit.

"""
Return an array of bytes representing an integer.

Internally relies on the python ``int.to_bytes()`` method.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a syntax to link Python methods, similar to :meth:`asdf`? Maybe more advanced devs like @tscrim will know

Copy link
Collaborator

@tscrim tscrim Feb 21, 2024

Choose a reason for hiding this comment

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

I am not sure. It might work as other :meth:`class_name.method_name` things, but that is more of a sphinx thing than anything sage-specific...

Copy link
Contributor

@grhkm21 grhkm21 left a comment

Choose a reason for hiding this comment

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

Otherwise it looks good to me, provided CI pass (missing the three ubuntu Conda ones)

vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 22, 2024
… `ZZ` and finite fields

    
I often have to work with the conversion of bytes / integers and
currently do this by working with `int` types and then casting things to
`ZZ` or elements of finite fields.

This PR is a small addition to `Integer` and `FiniteField` types which
allow this with simply functions which internally convert to / from
`int` for the user so that things can be done naively from SageMath
types.

There's a TODO in the code which acknowledges that a faster method for
`to_bytes` might be to work straight from the `gmp` object in cython for
the integers, but it wasn't obvious to me how to do this.

### Examples

```py
sage: ZZ.from_bytes(b'\x00\x10', byteorder='big')
16
sage: ZZ.from_bytes(b'\x00\x10', byteorder='little')
4096
sage: ZZ.from_bytes(b'\xfc\x00', byteorder='big', is_signed=True)
-1024
sage: ZZ.from_bytes(b'\xfc\x00', byteorder='big', is_signed=False)
64512
sage: ZZ.from_bytes([255, 0, 0], byteorder='big')
16711680
sage: type(_)
<class 'sage.rings.integer.Integer'>
```

```py
sage: (1024).to_bytes(2, byteorder='big')
b'\x04\x00'
sage: (1024).to_bytes(10, byteorder='big')
b'\x00\x00\x00\x00\x00\x00\x00\x00\x04\x00'
sage: (-1024).to_bytes(10, byteorder='big', is_signed=True)
b'\xff\xff\xff\xff\xff\xff\xff\xff\xfc\x00'
sage: x = 1000
sage: x.to_bytes((x.bit_length() + 7) // 8, byteorder='little')
b'\xe8\x03'
```

```py
sage: F = GF(65537)
sage: a = F.random_element()
sage: a.to_bytes()
b'\x00\n\x86'
sage: F.from_bytes(_)
2694
sage: a
2694
```

```py
sage: F = GF(3^10)
sage: a = F.random_element(); a
z10^8 + z10^7 + 2*z10^5 + 2*z10^4 + 2*z10^3 + z10^2 + z10 + 1
sage: a.to_bytes()
b'$\xf7'
sage: F.from_bytes(_)
z10^8 + z10^7 + 2*z10^5 + 2*z10^4 + 2*z10^3 + z10^2 + z10 + 1
```

### 📝 Checklist


- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation accordingly.
    
URL: sagemath#37343
Reported by: Giacomo Pope
Reviewer(s): Giacomo Pope, grhkm21, Travis Scrimshaw
Copy link

github-actions bot commented Mar 6, 2024

Documentation preview for this PR (built with commit e51474d; changes) is ready! 🎉

@vbraun vbraun merged commit 9991759 into sagemath:develop Mar 25, 2024
13 checks passed
@mkoeppe mkoeppe added this to the sage-10.4 milestone Mar 25, 2024
@GiacomoPope GiacomoPope deleted the encoding_to_bytes branch April 1, 2024 01:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants