Provide an Implicit I/O Convention #23
Conversation
This isn't how I wanted to implement this, but since our InstructionParser type doesn't currently support associated data (other than the instruction stream) this is the least-disruptive way of doing it. When we have time to work on a larger refactor, we can update the FileStreamer and FileStream types to support metadata for the streamed data, and store information about the circuit interface there.
I'm on the fence about this. On the one hand, it's a nice way to bake constant functionality into the Bristol fashion format, which doesn't support it (but probably should). On the other, it makes Reverie less flexible. It's possible to easily handle on the circuit/witness generation side, so the loss of flexibility is a completely avoidable downside. But it still annoys me to have to add extra constant bits to every witness we generate.
I don't expect we'll actually use this as it's rare that we get so lucky as to have all of our output wires in contiguous order without explicitly rewriting the wires or adding extra buffer gates for each output. Nonetheless, we can continue to use the explicit I/O convention so no harm done.
Wasn't getting built from the companion
companion/src/instruction/bristol.rs
Outdated
| } | ||
|
|
||
| impl FromStr for BristolHeader { | ||
| type Err = std::io::Error; |
There was a problem hiding this comment.
Using std::io::Error as the error type here is probably fine for now, but this is how you'd do it if you wanted to get rid of all of the incompatible-type unwrap calls within:
pub enum BristolHeaderError {
Io(std::io::Error),
ParseError(String),
}Then, you could impl std::error::Error and impl From<std::io::Error> for BristolHeaderError, and use BristolHeaderError as the Err type for impl FromStr.
(This thiserror crate automates a decent chunk of that).
There was a problem hiding this comment.
I've partially fixed this, but as option::NoneError doesn't implement Error (for reasons I don't really understand) I can't completely eliminate the unwrap calls from from_str.
There was a problem hiding this comment.
Yeah, that's a topic of some contention in Rust.
The "right" way to handle Options in a Result context is to map the Some(_) variant and convert the None variant into your own error type. For example:
let n_gate: usize = parts.next().unwrap().trim().parse()?;becomes (roughly, didn't verify):
let n_gate: usize = parts.next().map(|p| p.trim()).ok_or_else(|| CountParseError("couldn't parse gate count")).parse()?;where CountParseError is a new variant in your BristolHeaderError like so:
CountParseError(&'static str)Annoying that this doesn't work for NoneError, but oh well
This PR allows Reverie to parse circuits that follow the (suggested, but not required) convention used by Bristol fashion circuits of using the first
nwires for input bits and the finalnwires for output bits. It also updates the header parsing convention to accept the three-line Bristol fashion format as opposed to the two-line Bristol format format. It retains some degree of backwards compatibility with the old format as one may set the number of inputs and outputs to zero in the header and still manually provide INPUT/OUTPUT gates to the circuit.