-
Notifications
You must be signed in to change notification settings - Fork 15
Add ext_py matrices and vectors #262
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
base: master
Are you sure you want to change the base?
Changes from all commits
53ebf1b
32eb399
98a90b6
9facd7a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,43 @@ | ||||||||||
| name: Test ext_py | ||||||||||
|
|
||||||||||
| on: [push, pull_request] | ||||||||||
|
|
||||||||||
| jobs: | ||||||||||
| test: | ||||||||||
| if: ${{ github.event_name == 'push' || github.event.pull_request.head.repo.full_name != github.repository }} | ||||||||||
| runs-on: ubuntu-latest | ||||||||||
| env: | ||||||||||
| RUST_BACKTRACE: 1 | ||||||||||
| RUSTFLAGS: "-D warnings" | ||||||||||
|
|
||||||||||
| steps: | ||||||||||
| - uses: actions/checkout@v5 | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win Disable credential persistence on checkout.
🔒 Proposed fix - uses: actions/checkout@v5
+ with:
+ persist-credentials: false📝 Committable suggestion
Suggested change
🧰 Tools🪛 zizmor (1.26.1)[warning] 14-14: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false (artipacked) [error] 14-14: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy) (unpinned-uses) 🤖 Prompt for AI Agents |
||||||||||
|
|
||||||||||
| - name: Set up Python 3.14 | ||||||||||
| uses: actions/setup-python@v6 | ||||||||||
| with: | ||||||||||
| python-version: "3.14" | ||||||||||
|
|
||||||||||
| - name: Install rustup | ||||||||||
| id: rustup | ||||||||||
| uses: dtolnay/rust-toolchain@v1 | ||||||||||
| with: | ||||||||||
| toolchain: stable | ||||||||||
|
|
||||||||||
| - name: Cache files | ||||||||||
| uses: actions/cache@v5 | ||||||||||
| with: | ||||||||||
| path: | | ||||||||||
| ~/.cargo | ||||||||||
| /usr/share/rust/.cargo | ||||||||||
| **/target | ||||||||||
| key: ext-py-${{ steps.rustup.outputs.rustc_hash }}-${{ hashFiles('**/Cargo.*') }} | ||||||||||
|
|
||||||||||
| - name: Build wheel | ||||||||||
| run: cd ext_py && pip install --group dev && maturin build --out dist | ||||||||||
|
|
||||||||||
| - name: Install wheel and run tests | ||||||||||
| run: | | ||||||||||
| cd ext_py | ||||||||||
| pip install --force-reinstall dist/*.whl | ||||||||||
| pytest tests | ||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,139 @@ | ||
| use std::{ | ||
| fmt, | ||
| ops::{Add, Mul, Neg, Sub}, | ||
| }; | ||
|
|
||
| use super::{Fp, SmallFq, element::FieldElement}; | ||
| use crate::prime::ValidPrime; | ||
|
|
||
| /// A field element whose field type has been erased to one of the two concrete kinds the public | ||
| /// API exposes: a prime field [`Fp`] or a small extension field [`SmallFq`], each over a dynamic | ||
| /// [`ValidPrime`]. | ||
| /// | ||
| /// This lets callers that cannot be generic over the [`Field`](super::Field) trait — most notably | ||
| /// the Python bindings, where a `pyclass` cannot carry generic parameters — manipulate field | ||
| /// elements of either kind through a single type, with the arithmetic operators implemented once | ||
| /// here rather than re-matched at every call site. | ||
| /// | ||
| /// Combining two elements that do not live in the same field (a different kind, or the same kind | ||
| /// over a different prime/degree) is a genuine failure, not a value: the binary operators report it | ||
| /// as [`None`]/[`Err`] rather than silently coercing one operand or returning a default element. | ||
| #[derive(Clone, Copy, PartialEq, Eq, Hash, Debug)] | ||
| pub enum DynFieldElement { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So the only reason we can't just have DynField be the enum and then |
||
| Fp(FieldElement<Fp<ValidPrime>>), | ||
| SmallFq(FieldElement<SmallFq<ValidPrime>>), | ||
| } | ||
|
|
||
| /// Why [`DynFieldElement::try_div`] could not divide. | ||
| /// | ||
| /// Division has two distinct failure modes that callers may want to surface differently (the Python | ||
| /// bindings map them to `ValueError` and `ZeroDivisionError` respectively), so unlike the additive | ||
| /// operators this is reported with a dedicated enum rather than a bare [`Option`]. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| pub enum DivError { | ||
| /// The two operands do not live in the same field. | ||
| MismatchedField, | ||
| /// The divisor is the zero element. | ||
| DivisionByZero, | ||
| } | ||
|
|
||
| impl fmt::Display for DivError { | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| match self { | ||
| Self::MismatchedField => f.write_str("cannot combine elements from different fields"), | ||
| Self::DivisionByZero => f.write_str("division by zero"), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl std::error::Error for DivError {} | ||
|
|
||
| /// Implement an additive/multiplicative operator once, returning `None` when the operands are not | ||
| /// in the same field. The field-equality guard is necessary: [`FieldElement`]'s own operators use | ||
| /// the left operand's field unconditionally, so without it a mismatch would silently compute a | ||
| /// bogus result instead of signalling failure. | ||
| macro_rules! impl_binop { | ||
| ($trait:ident, $method:ident) => { | ||
| impl $trait for DynFieldElement { | ||
| type Output = Option<Self>; | ||
|
|
||
| fn $method(self, rhs: Self) -> Self::Output { | ||
| match (self, rhs) { | ||
| (Self::Fp(a), Self::Fp(b)) if a.field() == b.field() => { | ||
| Some(Self::Fp(a.$method(b))) | ||
| } | ||
| (Self::SmallFq(a), Self::SmallFq(b)) if a.field() == b.field() => { | ||
| Some(Self::SmallFq(a.$method(b))) | ||
| } | ||
| _ => None, | ||
| } | ||
| } | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| impl_binop!(Add, add); | ||
| impl_binop!(Sub, sub); | ||
| impl_binop!(Mul, mul); | ||
|
|
||
| impl Neg for DynFieldElement { | ||
| type Output = Self; | ||
|
|
||
| fn neg(self) -> Self::Output { | ||
| match self { | ||
| Self::Fp(x) => Self::Fp(-x), | ||
| Self::SmallFq(x) => Self::SmallFq(-x), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl DynFieldElement { | ||
| /// Divide, distinguishing a mismatched-field failure from division by zero. Returns | ||
| /// [`DivError::MismatchedField`] when the operands are not in the same field and | ||
| /// [`DivError::DivisionByZero`] when `rhs` is the zero element. | ||
| pub fn try_div(self, rhs: Self) -> Result<Self, DivError> { | ||
| match (self, rhs) { | ||
| (Self::Fp(a), Self::Fp(b)) if a.field() == b.field() => { | ||
| (a / b).map(Self::Fp).ok_or(DivError::DivisionByZero) | ||
| } | ||
| (Self::SmallFq(a), Self::SmallFq(b)) if a.field() == b.field() => { | ||
| (a / b).map(Self::SmallFq).ok_or(DivError::DivisionByZero) | ||
| } | ||
| _ => Err(DivError::MismatchedField), | ||
| } | ||
| } | ||
|
|
||
| /// The multiplicative inverse, or [`None`] for the zero element. | ||
| pub fn inv(self) -> Option<Self> { | ||
| match self { | ||
| Self::Fp(x) => x.inv().map(Self::Fp), | ||
| Self::SmallFq(x) => x.inv().map(Self::SmallFq), | ||
| } | ||
| } | ||
|
|
||
| /// The Frobenius endomorphism `x -> x^p`. | ||
| pub fn frobenius(self) -> Self { | ||
| match self { | ||
| Self::Fp(x) => Self::Fp(x.frobenius()), | ||
| Self::SmallFq(x) => Self::SmallFq(x.frobenius()), | ||
| } | ||
| } | ||
|
|
||
| /// The canonical `u32` value of a prime-field element, or [`None`] for a [`SmallFq`] element, | ||
| /// which has no canonical integer representative. | ||
| pub fn try_as_u32(self) -> Option<u32> { | ||
| match self { | ||
| Self::Fp(x) => Some(*x), | ||
| Self::SmallFq(_) => None, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl fmt::Display for DynFieldElement { | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| match self { | ||
| Self::Fp(x) => write!(f, "{x}"), | ||
| Self::SmallFq(x) => write!(f, "{x}"), | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,2 @@ | ||||||||||||||
| __pycache__ | ||||||||||||||
| *.so | ||||||||||||||
|
Comment on lines
+1
to
+2
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win Add
♻️ Proposed addition __pycache__
*.so
+target/
+dist/📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| [package] | ||
| name = "ext_py" | ||
| version = "0.1.0" | ||
| edition = "2021" | ||
|
|
||
| # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html | ||
| [lib] | ||
| name = "ext_py" | ||
| crate-type = ["cdylib"] | ||
|
|
||
| [dependencies] | ||
| anyhow = "1.0" | ||
| fp = { path = "../ext/crates/fp" } | ||
|
|
||
| pyo3 = "0.29.0" | ||
| serde_json = "1.0.141" | ||
|
|
||
| [dev-dependencies] | ||
| tempfile = "3.20.0" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| [build-system] | ||
| requires = ["maturin>=1.8"] | ||
| build-backend = "maturin" | ||
|
|
||
| [project] | ||
| name = "sseq_ext" | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is that the name we're settling on? I suppose it fits. |
||
| requires-python = ">=3.10" | ||
| classifiers = [ | ||
| "Programming Language :: Rust", | ||
| "Programming Language :: Python :: Implementation :: CPython", | ||
| "Programming Language :: Python :: Implementation :: PyPy", | ||
| ] | ||
| dynamic = ["version"] | ||
|
|
||
| [tool.maturin] | ||
| features = ["pyo3/extension-module"] | ||
| module-name = "ext" | ||
| python-source = "python" | ||
|
|
||
| [dependency-groups] | ||
| dev = [ | ||
| "maturin>=1.8", | ||
| "pytest", | ||
| ] | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| from .ext import * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Add a least-privilege
permissions:block.The workflow inherits the repository's default
GITHUB_TOKENpermissions. Scope it down explicitly, since this job only checks out and builds/tests.🔒 Suggested change
jobs: test: if: ${{ github.event_name == 'push' || github.event.pull_request.head.repo.full_name != github.repository }} runs-on: ubuntu-latest + permissions: + contents: read env: RUST_BACKTRACE: 1 RUSTFLAGS: "-D warnings"📝 Committable suggestion
🧰 Tools
🪛 zizmor (1.26.1)
[info] 6-6: workflow or action definition without a name (anonymous-definition): this job
(anonymous-definition)
🤖 Prompt for AI Agents
Source: Linters/SAST tools