-
Notifications
You must be signed in to change notification settings - Fork 48
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
feat: generic circuit struct #123
Closed
brech1
wants to merge
11
commits into
privacy-scaling-explorations:threading-refactor
from
brech1:feat/generic-circuit
Closed
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
12eb964
feat: generic circuit
brech1 242a1c9
docs: circuit module title
brech1 6b76ffc
docs: fix comments
brech1 df4b074
fix: model
brech1 599f66c
add: example binary impl
brech1 8947554
remove execution
brech1 db13e8e
implement sorting
brech1 e1cea18
fmt and fix tests
brech1 cf4f3a5
implement required changes
brech1 3ac77dd
update
brech1 8ed2ac2
remove generic node
brech1 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,5 @@ | ||
pub mod circuit; | ||
pub mod model; | ||
mod circuit; | ||
mod model; | ||
|
||
pub use circuit::{Circuit, CircuitBuilder, CircuitBuilderError}; | ||
pub use model::{Component, Node}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,11 +2,24 @@ | |
//! | ||
//! This module contains the main traits and structures used to represent the circuits. | ||
|
||
/// A `Component` trait that represents a block with inputs and outputs. | ||
/// A `Component` defines a block with inputs and outputs. | ||
pub trait Component { | ||
/// Returns the input node indices. | ||
fn get_inputs(&self) -> Vec<usize>; | ||
/// Returns an iterator over the input node indices. | ||
fn get_inputs(&self) -> impl Iterator<Item = &Node<u32>>; | ||
|
||
/// Returns the output node indices. | ||
fn get_outputs(&self) -> Vec<usize>; | ||
/// Returns an iterator over the output node indices. | ||
fn get_outputs(&self) -> impl Iterator<Item = &Node<u32>>; | ||
} | ||
|
||
/// A circuit node, holds a generic type identifier. | ||
#[derive(Debug, Eq, PartialEq)] | ||
pub struct Node<T> { | ||
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. Why is this generic? |
||
pub(crate) id: T, | ||
} | ||
|
||
impl<T> Node<T> { | ||
/// Creates a new node with the given identifier. | ||
pub fn new(id: T) -> Self { | ||
Self { id } | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Can we add a
Node
newtype, as we currently have inmpz-circuits
. Gives us better typing. Perhaps we should also switch it fromusize
tou32
so the circuit isn't needlessly twice the size on 64 bit archs.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.
Node
type is not strictly necessary so I wouldn't implement it in the model. Don't see how much of an improvement this extra typing could provide on our level, perhaps implementers could add this layer themselves.usize
in the end, on 64-bit architectures you'll have to complete the address to access an index.But both are good suggestions, I could be missing something!
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.
It's all about the public API: What do you want to commit to? Using
usize
commits to that underlying representation, whereas aNode(u32)
type allows us to constrain the data type and the traits which a node implements. For example why would a node implementAdd
?usize
does. Do we want to allow it to be mutated? How might that affect certain invariants we have elsewhere? This is less an issue for private types, but this is explicitly part of the crates pub api.Yes it will be used to index into linear memory, but a
Vec<u32>
is half the size of aVec<u64>
, and hence aVec<Gate>
will be half the size ifNode
is backed by au32
instead ofu64
on 64-bit archs.