-
Notifications
You must be signed in to change notification settings - Fork 9
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: Implement LaTeX generation #145
base: main
Are you sure you want to change the base?
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
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.
Left some comments on how to make this more Rusty.
I'd also like to suggest looking into the builder pattern in Rust, which is (1) more conventional than a Java-like Factory setup and (2) may help avoid errors from bad user input by just not allowing it.
This reverts commit a25abb4.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Co-authored-by: Kalan <22137047+kalzoo@users.noreply.github.com>
This reverts commit 04c2abf.
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.
This is looking really good and I like how things have simplified and been split into modules with their own concerns.
Some high level suggestions, some of which are repeats of comments I left:
First, make sure you run cargo clippy --all-features
-- there are some things like format!("{}", value)
that would be caught by it.
Second, there's an odd mix of to_latex
and fmt::Display
, which leads to things like erasing errors that may be useful to the end user. My suggestion for this is to follow a pattern I've seen in a few places, where you have the "real" struct
, e.g. QuantikzCell
, and a function that creates a new struct, e.g. QuantikzCellDisplay<'a>
, which is known to be in a good state and implements fmt::Display
. This:
- Helps decrease the number of owned strings being passed around, which should optimize the runtime a little
- Separates the validation of the input from printing the output, so there's no more replacing a useful error with
fmt::Error
- The benefit of this over a function that combines actual writing with validation is that all validation happens before any files get modified
- Can be combined to build larger structs that also implement
fmt::Display
, e.g.struct ProgramDisplay<'a> { cells: Vec<QuantikzCellDisplay<'a>>, // etc. }
f, | ||
"{}{}", | ||
&RenderCommand::Separate, | ||
command.to_latex(&cell_settings).map_err(|_| fmt::Error)? |
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.
I'm not a fan of erasing errors like this. See my note above about having a display_latex
function, which avoids this.
From the docs for std::fmt::Error
:
This type does not support transmission of an error other than that an error occurred. Any extra information must be arranged to be transmitted through some other means.
IMO, the only error that should come from Display
is to do with the actual writing of text, not validating the thing to write
Co-authored-by: Michael Bryant <shadow53@shadow53.com>
That's strange, doesn't look like clippy catches the issues you mentioned @Shadow53. I have clippy --fix set automatically whenever I save. I also ran |
Odd. Might be something that I'm used to having enabled in my projects that isn't enabled by default or in |
@Shadow53 I haven't heard from anyone on the team on the feedback you requested from them on this. Is this something worth pursuing on this PR? |
This draft PR transfers the pyQuil LaTeX generation feature to quil-rs.
The initial commit,
latex.rs
to theprogram
crate.Cargo.toml
andmod.rs
.CONTROLLED H 3 2
.program/latex.rs
This file has two sections with some initial code,
LaTeX Implementation
The LaTeX implementation section of
latex.rs
is where the LaTeX generation will occur. The current template was used to generate the first snapshot test. It includes DiagramSettings, directly taken from pyQuil, that will allow users to customize the LaTeX for their Program following the Builder design pattern in Rust. The template also contains an outline for displaying variousResult::Err
using thiserror on aLatexGenError
enum.Testing Suite
The first test in the testing module containing the suite of LaTeX unit tests is a simple snapshot test for
CONTROLLED H 3 2
. The snap file for this test is program/snapshots/quil_rs__program__latex__tests__controlled_gate.snap. It was generated by passing the following expected LaTeX to the unit testtest_controlled_gate
as the initial snapshot assertion.Main Tasks and Sub Tasks lists will be used to help keep track of the work. Once all tasks are marked as complete in the Main Tasks the stage of this PR can be changed from draft to ready for review.
Main Tasks
Sub Tasks (live list)
I. Implement all LaTeX generation features currently available in pyQuil.
test_controlled_gate
test.II. Implement new CCNOT and CPHASE features
*Canonical Forms
CNOT
->CONTROLLED X
CCNOT
->CONTROLLED CONTROLLED X
CPHASE
->CONTROLLED PHASE