Fix EXPT printing #257

appleby · 2019-05-24T15:26:05Z

This is a WIP, but I wanted to get early feedback to see how folks feel about the "golden file" testing approach used here, before I go crazy and add a bunch of tests.

Assuming no one hates the golden file approach, my plan was to write a small script that can be run to update the expected outputs whenever changes to the printer are made, and then to add a bunch of these tests (and probably some regular lisp-based tests to handle any edge-cases that are otherwise hard to tickle).

Pros of golden files:

Easy to add new test cases
Easy to automate updating expected output if the printer changes

Cons of golden files:

Have to remember to update expected output if the printer changes
Easy to automate updating expected output if the printer changes, and then blissfully ignore wrong updated output if the diff is not scrutinized sufficiently closely.

This is not really specific to the "golden file" approach, but basically whether or not it makes to add a bunch of printer tests depends on how stable the printed representations are. If they're constantly changing, then updating the test expectations will come to be a tedious chore. If they're relatively stable, only a minor tedious chore!

Determine whether golden files considered harmful
IF golden files not hateful THEN write program to automate regeneration of golden files
Add more tests
Simplify handling of trailing newlines
Add single-source-of-truth for bi-directional mapping between quil arthimetic operators & functions <--> lisp functions to prevent bugs when/if quil functions are added/removed. Note that *valid-functions* already exists in parser.lisp, but does not include the mappings for infix arithmetic ops.
Move generic lisp types and type predicates like string-list-p into a new types.lisp file
Write actual commit messages
Profit!

Fixes #249

appleby · 2019-05-24T15:28:43Z

src/ast.lisp

@@ -112,7 +112,7 @@ EXPRESSION should be an arithetic (Lisp) form which refers to LAMBDA-PARAMS."
  "Evaluate the delayed expression DE to a numerical value (represented in a CONSTANT data structure). MEMORY-MODEL is an association list with keys MEMORY-REF structures and values the value stored at that location."
  (labels ((lookup-function (expr)
             (case expr
-               ((+ - * / cos sin tan) expr)
+               ((+ - * / expt cos sin sqrt exp cis) expr)


I assume that this list of functions should match what is accepted by the parser?

That seems reasonable. But it seems so reasonable that I'm surprised this wasn't caught earlier -- so maybe there is something we're both missing. @ecpeterson @stylewarning

This is correct. I’m surprised (1) that Quil uses expt to express exponentials, rather than ^ or **, and (2) that tan isn’t legal Quil. Shows how carefully I read!

It does use ^

Correct. Quil uses infix "^", which maps to cl:expt on the lisp side. This is actually the only case where the name of the Quil operator and the lisp function differ, and was the source of this bug.

My idea is to add some abstraction as part of this PR to provide a single source of truth for the quil <--> lisp mapping, so that this case form won't have to duplicate the list of valid functions here.

appleby · 2019-05-24T15:32:19Z

tests/printer-test-files/delayed-expressions.quil

+    RX((%arg1-%arg2)) p
+
+
+RX(2.141592653589793) 0


Not sure how portable it is to depend on the default printed representation for ~F here.

That's a very good point and worth more investigation, considering that we're going to be supporting ECL soon enough.

how about doing TEST(pi, pi*SIN(pi/2))

I almost did precisely that before opening the PR, but figured it was better to have the conversation here since I thought it my crop up again.

appleby · 2019-05-24T15:39:10Z

Forgot to mention that another approach to the golden files is to have inputs and expected outputs live in separate files, with at most one input/output per file. Then parse-golden-file can go away. That approach is simpler and more machine-friendly, but I feel like the ability to have multiple inputs/outputs in a single file is more human-editor friendly. Debatable, though.

notmgsk · 2019-05-24T16:22:57Z

Forgot to mention that another approach to the golden files is to have inputs and expected outputs live in separate files, with at most one input/output per file. Then parse-golden-file can go away. That approach is simpler and more machine-friendly, but I feel like the ability to have multiple inputs/outputs in a single file is more human-editor friendly. Debatable, though.

If we're going the golden-file route, then my vote is for keeping input/output in the same file for exactly the reason you noted. I'll defer my vote on this golden-file idea for now.

@appleby you'll have to test the golden test stuff right? What do we call that? Very golden test?

notmgsk · 2019-05-24T16:31:46Z

Then again, relying on parse-golden-file introduces more surface area for bugs. I'm on the fence.

appleby · 2019-05-24T16:35:26Z

@appleby you'll have to test the golden test stuff right? What do we call that? Very golden test?

Ha! Platinum test files? Meta-Golden?

Actually I did consider writing some tests for parse-golden-file, but then thought maybe testing the tests was overkilll. The "parser" is quite simple, probably could be replaced with a regex or even a "split on substrings" function if I could find one (anyone? anyone?), plus the input file format is unlikely to change much, so I've just repl-tested it for now. But I'm happy to add some tests for parse-golden-file, if desired.

Actually, one reason I went with a simple parser rather than chunk-on-substrings is I had the idea that it could maybe be extended to also handle inputs that are meant to signal error via another magic comment cookie like "# Signals: foo-error" or the like.

appleby · 2019-05-24T16:39:33Z

tests/utilities.lisp

+          :with inputs := '()
+          :with outputs := '()
+          :for line-number :upfrom 1
+          :for line :in (split-sequence:split-sequence


This probably qualifies as gratuitous consing, but I'm being being lazy for now. I might switch to read-line and just deal with edge cases myself.

IMO gratuitous is fine in the test suite.

i wouldn't even mind if you did

:for line :in (loop :repeat 1000 :do (cons 'appleby 'appleby) :finally (return (split-sequence ...

Tempting...

This is now fixed because, in the words of the Elephant Man: I AM NOT AN ANIMAL!

notmgsk · 2019-05-24T16:46:28Z

I wonder if this parsing business could be improved by marking gates with a pragma instead. E.g PRAGMA TEST_{INPUT,OUTPUT} or PRAGMA BEGIN_TEST_INPUT_BLOCK etc. cl-quil ought to have some pragma block logic you can co-opt (someone has already implemented PRAGMA PRESERVE_BLOCK ... PRAGMA END_PRESERVE_BLOCK).

appleby · 2019-05-24T17:02:56Z

I wonder if this parsing business could be improved by marking gates with a pragma instead. E.g PRAGMA TEST_{INPUT,OUTPUT} or PRAGMA BEGIN_TEST_INPUT_BLOCK etc. cl-quil ought to have some pragma block logic you can co-opt (someone has already implemented PRAGMA PRESERVE_BLOCK ... PRAGMA END_PRESERVE_BLOCK).

Personally, I would rather not sully the real parser with testing concerns if it can be avoided. Unless it's something that is expected to be generally useful elsewhere? Or are you saying there is some pre-existing PRAGMA facility that I could use instead?

notmgsk · 2019-05-24T17:08:30Z

I wonder if this parsing business could be improved by marking gates with a pragma instead. E.g PRAGMA TEST_{INPUT,OUTPUT} or PRAGMA BEGIN_TEST_INPUT_BLOCK etc. cl-quil ought to have some pragma block logic you can co-opt (someone has already implemented PRAGMA PRESERVE_BLOCK ... PRAGMA END_PRESERVE_BLOCK).

Personally, I would rather not sully the real parser with testing concerns if it can be avoided. Unless it's something that is expected to be generally useful elsewhere? Or are you saying there is some pre-existing PRAGMA facility that I could use instead?

I expected that block logic to exist elsewhere, based on the existence of PRAGMA (END)?_PRESERVE_BLOCK. Seems to be not the case, however.

ecpeterson · 2019-05-24T17:22:39Z

I, too, think that there should be some support for “region” PRAGMAs. I think there was an internal ticket for this, but it didn’t survive the public migration, & it isn’t very clear to me how it ought to be done anyhow.

stylewarning · 2019-05-24T18:39:24Z

(this thread has used the word golden too much and has been semantically satiated)

stylewarning · 2019-05-24T18:54:23Z

tests/printer-tests.lisp

+      (multiple-value-bind (golden-inputs golden-outputs) (parse-golden-file file)
+        (loop :for input :in golden-inputs
+              :for expected-output :in golden-outputs :do
+                (let* ((input-pp (quil:parse-quil input))


@ecpeterson @notmgsk @appleby Should we test parse-quil or parse-quil-into-raw-program?

https://i.imgur.com/X1mVtuY.png

I forgot to ask about this. I was considering doing both in a loop, but not sure if there is any point in that.

I don't think it matters

Maybe it does if parse-quil is simplifying arithmetic

I think the simplification is happening @ parse time, not @ transformation time.

Unless there's a specific reason to choose the other, I'd prefer parse-quil, since it's what's used more commonly elsewhere.

In the latest round of commits, I've added keyword options to parse-and-print-quil-to-string that allows the caller to specify the parser and printers they want, defaulting to parse-quil and print-parsed-program, respectively.

The golden file tests are still using the default parse-quil. I tend to agree with @ecpeterson that parse-quil appears to be the user-visible exported interface and also winds up testing more code paths since it does the standard transforms. So far, I haven't run into any test cases where that is a problem.

stylewarning · 2019-05-24T18:57:52Z

tests/utilities.lisp

+          :for line :in (split-sequence:split-sequence
+                         #\Newline
+                         (alexandria:read-stream-content-into-string stream))
+          :do (progn


can nix the progn

Good eye! Done.

stylewarning · 2019-05-24T18:58:07Z

tests/utilities.lisp

+                         (alexandria:read-stream-content-into-string stream))
+          :do (progn
+                (ecase state
+                  (:start


can you capitalize the :STATEs?

stylewarning · 2019-05-24T18:58:18Z

tests/utilities.lisp

+                  (:start
+                   (unless (string= line "# Input")
+                     (parse-error line-number line "Expected ~S" input-header))
+                   (setf state :reading-input))


quote keywords here and elsewhere ':reading-input

Done. I noticed this pattern elsewhere in the code. Why quote a self-quoting symbol?

indicate that it's data, and not a keyword argument

self-evaluating? you know what I mean...

stylewarning · 2019-05-24T18:59:58Z

tests/utilities.lisp

+  (with-open-file (f file-name)
+    (parse-golden-file-stream f :strip-final-blank-line strip-final-blank-line)))
+
+(define-condition golden-file-parse-error (alexandria:simple-parse-error)


could you put this above these functions?

Done. I actually moved this down to the bottom just before opening this PR because I was hoping the documentation string for parse-golden-file-stream would serve as a lazy-man's section intro comment.

tests/utilities.lisp

appleby · 2019-05-27T17:14:30Z

Marking this DO NOT REVIEW until I finish the remaining tasklist.

stylewarning · 2019-05-28T18:41:54Z

tests/printer-tests.lisp

+(deftest test-print-instruction ()
+  (is (string= "PRAGMA gate_time CNOT \"50 ns\""
+               (with-output-to-string (s)
+                 (cl-quil::print-instruction (make-instance 'quil::pragma


here's a Cool Tip, you can do

(format s "~/cl-quil:instruction-fmt/" x)

to print the x instruction to stream s. Use s = NIL to create a string.

Nice. This was actually a pre-existing test that I moved from another file, but I will update it as part of this PR.

stylewarning · 2019-05-28T18:43:20Z

tests/utilities.lisp

+ADD-FINAL-NEWLINE matches the value you plan to pass for STRIP-FINAL-NEWLINE when parsing the golden
+file in your tests. Both default to T.
+
+IF-EXISTS has the standard common lisp meaning. See http://l1sp.org/cl/open."


Suggested change

IF-EXISTS has the standard common lisp meaning. See http://l1sp.org/cl/open."

IF-EXISTS has the standard Common Lisp meaning. See http://l1sp.org/cl/open."

tests/utilities.lisp

appleby · 2019-05-28T20:24:49Z

tests/utilities.lisp

+            (and (typep object ',container-type)
+                 (every #',element-test object))))))
+  (def string-list-p list stringp)
+  (def string-sequence-p sequence stringp))


There are a handful of other places that similar type predicates to these are defined throughout the code. For instance, there is integer-sequence-p in define-pragma.lisp and integer-list-p in chip-reader.lisp.

I considered collecting them all together in some new file named like types.lisp or some such, along with whatever other general-purpose type-related predicates or type aliases I might find.

On the one hand, it's nice to have them defined in the same file where they are used, and for highly-specific types that seems like the best way to go, but for general-purpose "list/sequence of foo" types like this, there is also something to be said for having them live in a single source file for easy discovery.

I think the last paragraph is a sound idea. optimal-2q-target-atom (or whatever it's called) doesn't belong in types.lisp, but string-list-p sure would.

Ok, I've added it to my todo list, above. I will scrounge around in the sources and see what other generic types / type predicates I can find and move them into a new file. Anyone care what the file is called?

types.lisp

lisp-types.lisp

generic-types.lisp

generic-lisp-types.lisp

types-of-the-generic-persuasion-that-pertain-to-lisp-usage-but-certainly-not-quil-types.lisp

I like types.lisp just fine :) but whatever you like.

appleby · 2019-05-29T14:36:29Z

tests/printer-tests.lisp

+  represented by singleton classes. This is because the **COMMENTS** hash table is keyed on object
+  identity, so if any prior tests have attached rewiring comments to any singleton instructions,
+  they will persist and be printed by PRINT-PARSED-PROGRAM."
+  (setf quil::**comments** (quil::make-comment-table)))


This should probably be fixed rather than hacked around, but I'm not sure at glance how to tackle it, so I prefer to do it in a follow-on PR.

I've opened #270 to track fixing this.

appleby · 2019-05-29T14:40:43Z

tests/utilities.lisp

+
+If you call this function, examine the diffs of the old vs new output sections *carefully* before
+committing the updated files, in order to ensure that all the changes are intended or expected.
+Golden files are precious, and their sanctity must be preserved!


I have tried to follow my own advice here and examine the "expected output" sections of the new golden files carefully to make sure they are correct, but I may have missed something. Extra care from reviewers in verifying that the new golden files contain correct outputs is greatly appreciated.

appleby · 2019-05-29T14:45:40Z

tests/printer-test-files/basics.quil

+H 1
+RX(pi) 0
+RY(-1.0) 1
+RZ(pi/2) 2


Note that this pi/2 has not been evaluated to a float, but an expression like 2*pi does get evaluated to 6.283185307179586. Expected? I assumed this was a readability convenience since fractional pi angles are probably common, but haven't looked into the source of it yet.

The parser evaluates things to floats when it can, and (by default, though this is disable-able) we print out things that are near-ish to particular rational multiples of pi as such, since the (default) human user probably finds that helpful. The relevant flag is *print-fractional-radians*.

I'd indeed expect 2pi to be on such rational multiple of pi that we choose to pretty-print.

I will make a note to look into this.

appleby · 2019-05-29T14:53:45Z

tests/printer-test-files/classical.quil

+STORE b int[0] b[0]
+
+# Input
+# Stolen from tests/good-test-files/good-classical-binaries.quil


I am considering adding a function to pilfer golden inputs from other quil test files in the test suite (good-test-files, compiler-hook-test-files). The idea is that the function could be called interactively and would copy the quil source from those other test files and turn them into golden printer tests. I would call it something like plunder-gold-from-yonder-files (working title).

Does this sound like a good idea or overkill? What say ye?

Yea!

Nay!

Meh.

I think the thumbs-up means this has one "Yea!" vote. I assume no response from the other reviewers is an implicit "Meh." vote :).

Thinking about it more, I'm not sure this really needs to be an automated process, depending on how often tests cases get added to the other tests. I'm also not sure all of the test cases from those other directories will map to reasonable printer tests (might contain expressions that evaluate to crazy floats, for example).

This might be a "solution in search of a problem". But there's no denying I really want to write a function called plunder-gold-.... I will do a closer review of the test cases in question and circle back.

My pirate's raid to plunder golden inputs has yielded a potential bug! See #272.

I think this PR is big and hairy enough as it is (100+ comments!), so I've spun the idea of plundering gold out into a separate issue (#273) that can happen in a follow-on PR, assuming we decide it's worthwhile to do it.

I agree, we should close this PR off and do follow ups

appleby · 2019-05-29T15:19:27Z

tests/printer-test-files/decomposed-ccnot.quil

+    DAGGER T q2
+    CNOT q1 q2
+
+DECOMPOSED-CCNOT 0 1 2


Calling @ecpeterson

So the current test is just parsing and printing the above Quil and making sure it does the expansion of the DEFCIRCUIT.

But I also noticed that the compiler is awesome and does indeed compile this DECOMPOSED-CCNOT into the equivalent of good ol' CCNOT 0 1 2, by which I mean the matrices returned by parsed-program-to-logical-matrix for the two programs are matrix-equal-dwim.

In fact, in this case, not only are the logical matrices "equal" but compiler-hook emits a nearly identical instruction sequence. (I think "equivalent up to phase" is the phrase I want? Not sure. Basically, the instruction sequences are identical, modulo the %theta parameters to the R* gates). The fact that the logical matrices were equal didn't surprise me (I guess that means the compiler is working!), but the fact that the instruction sequences were nearly identical did. But maybe the one implies the other?

Anyway, getting to the point: I looked around for tests that compare the output of compiler-hook when given two equivalent programs but with different Quil sources, but didn't find any such tests. Would it make sense to add some (assuming I didn't just miss them), or did I get lucky with CCNOT and DECOMPOSED-CCNOT?

It seems I did not search carefully enough. It looks like compiler-hook-tests.lisp has a handful of such tests, e.g. test-parsed-program-to-logical-matrix-cnot-rewiring, test-parsed-program-to-logical-matrix-swap-rewiring, etc.

I take it back. The above mentioned tests seem to be concerned specifically with rewirings, and don't pass the parsed-programs through compiler-hook.

I think the similarity in the instruction sequences that you've noticed is partially a matter of luck and partially a matter of implementation decisions. The compiler is typically in one of two situations: it either knows exactly what to do (e.g., it recognizes a CCNOT and that it has a decomposition for CCNOT in its back pocket) or it has no idea what to do (e.g., you supply it a random gate, and so it relies on some perfectly generic scheme). It's pretty rare that, during the course of a single compilation, you jump from Scenario A to Scenario B—i.e., the execution paths are usually either all A or all B. In either case, you're likely to get a similar pattern of gates out (up to the precise value of parameters, which gets influenced by all sorts of things, from numerical jitter to LAPACK's decision to sneak an extra reflection into a matrix decomposition for no particular reason).

It's not a bad idea to implement the test you mention, where you check that two distinct instruction sequences encode the same unitary, though I think it'd primarily be a check that make-matrix-from-quil is performing as expected—which is certainly in the critical path, but it's also pretty low-risk.

N.B.: "Equivalent up to phase" means that the two unitaries U, V encoded by the two instructions / instruction sequences have the property U = cV for some complex scalar c. In some sense, this definition compares with the parameter situation in the opposite way to what you describe: the more efficient a circuit template (i.e., gate sequence w/o prespecified parameters) is at encoding different unitaries, the harder it will be to find different choices of angle values that give the same total unitary up to phase. Irredundancy pushes you in the other direction.

Haha. So... the exact opposite of what I said. Got it! ;-)

Let me see if I can clarify, and I'll open an issue to track this idea.

So do I understand correctly that what you are saying is that two semantically equivalent Quil programs with different sources will not necessarily compile down to parsed-programs whose logical matrices satisfy matrix-equality (or even matrix-equal-dwim)? And instead the valid test would be merely parse the programs, then pass their instruction sequences to make-matrix-from-quil and check if the resulting matrices were equal?

In other words, this is bogus:

(quil::matrix-equality (quil::parsed-program-to-logical-matrix (quil:compiler-hook (quil:parse-quil program-a) some-chip-spec)) (quil::parsed-program-to-logical-matrix (quil::compiler-hook (quil:parse-quil program-b) some-chip-spec)))

But this should work:

(quil::matrix-equality (quil::make-matrix-from-quil (coerce (quil::parsed-program-executable-code (quil:parse-quil program-a)) 'list)) (quil::make-matrix-from-quil (coerce (quil::parsed-program-executable-code (quil:parse-quil program-b)) 'list)))

matrix-equality isn't guaranteed work (in either case!), but matrix-equals-dwim (or operator=) will work. The names of these predicates could stand to be improved, but here's what they do:

double= is used to test (complex) float equality throughout the code, except for when checking output.

double~ is used to test (complex) float equality on the output matrices. Its tolerance threshold is greater than that of double=: double= gets used to make thousands of substitution decisions through the course of a quilc run, and so double= is thousands of times more sensitive than double~ to make up for it.

matrix-equality: Tests whether A is entrywise double~ to B.

operator=: Checks whether there is a complex number c (the "global phase difference") so that c*A and B are entrywise double~. (This is the definition of operational equality in quantum mechanics; it's a fact of life that quantum operations admit their 'easiest' expression in terms of matrices, but in trade for this easy expression you have to use this clumsy notion of equality.)

matrix-equals-dwim: The same as operator=, except when *enable-state-prep-compression* is turned on, in which case the compressor is only guaranteed to emit a sequence of instructions whose corresponding operator passes operator= only with the leftmost column of the input matrix, and so matrix-equals-dwim tests only this column when this flag is high.

The last one is usually what you want.

While we're at it: parsed-program-to-logical-matrix is make-matrix-from-quil + sensitivity to rewiring comments. This extra caveat means that parsed-program-to-logical-matrix will always emit the same matrix for Quil passed through the addresser; all the other subroutines in quilc don't require this extra sensitivity, and m-m-f-q will work just fine.

As usual, thank you for your patient explanation. I will go ahead and open an issue to track this idea, although I suspect I will struggle to come up with many examples of programs with different sources but equivalent "meaning". But that sounds like a fun exercise for me to get more familiar with Quil and quantum programming.

You can get a head start by looking at rewriting-rules.lisp, which is full of little not-obviously-equal Quil snippets that are equivalent to each other in the matrix-equals-dwim sense.

This is now #274

appleby · 2019-06-04T19:54:33Z

Also, now there are merge conflicts. RIP.

I was anticipating this, so I waited till after lunch.

Ensure that the lisp function EXPT maps to the Quil infix operator "^" when printing a DELAYED-EXPRESSION in PRINT-INSTRUCTION-GENERIC. Fixes quil-lang#249

- Move existing PRINT-INSTRUCTION tests into a new file tests/printer-tests.lisp - Add support for testing "golden files", which are tests files that contain a sequence of input/expected-output pairs. See the comment in tests/utilities.lisp for more info. - Add a bunch of new PRINT-INSTRUCTION tests using the new golden file format.

Rename the test and replace all (with-output-to-string (s) (cl-quil::print-instruction ... s)) with (format nil "~/cl-quil:instruction-fmt/" ...). Updates quil-lang#116

Resetting the **COMMENTS** table might be required for any test that wants to validate the output of PRINT-PARSED-PROGRAM for programs that contain HALT, NOP, RESET, or other instructions that are represented by singleton classes. This is because the **COMMENTS** hash table is keyed on object identity, so if any prior tests have attached rewiring comments to any singleton instructions, they will persist and be printed by PRINT-PARSED-PROGRAM. See also "Curio No. 2: Comment sharing" from quil-lang#245

The purpose of this commit is to add single source of truth + some abstraction for how Quil functions and operators map to a lisp symbols and vice versa. The motivation for this change is that previously there were a handful of places were the logic for validating and/or translating between Quil and lisp was duplicated, which led to bugs when the list of valid functions, say, was updated in one place but not the other.

Mention that printer-test-files/decomposed-ccnot.quil was stolen from Nielsen & Chuang.

Allow the caller of PARSE-AND-PRINT-QUIL-TO-STRING to specify the :PARSER and :PRINTER they want, and make sure to use this function everywhere applicable in printer-tests.lisp.

- Allow a golden test case to disable the fixed-point check for a particular output by leaving a comment in the corresponding Input section. - Add a golden test file defgates.quil that contains an example of an input/output pair for which the fixed-point check needs to be disabled. - Move one of the test cases that previously appeared in TEST-DEFGATE-PRINTING into delayed-expressions.quil. - Add comments explaining why the remaining test cases in TEST-DEFGATE-PRINTING and TEST-CIRCUIT-AND-DECLARE-PRINTING are not simply included in the golden parse/print tests. - Update TEST-DEFGATE-PRINTING with a NOT-SIGNALS fiasco assertion.

- Add GOLDEN-TEST-CASE structure to represent individual input/output pairs parsed from a golden test file. This structure also tracks the originating file name and line number for each test, as well as the input and output sections. - PARSE-GOLDEN-FILE{,-STREAM} now returns a list of GOLDEN-TEST-CASE structures. - TEST-PRINT-PARSED-PROGRAM-GOLDEN-FILES now includes the file name and line number of the input/output pair that caused a failure for any failing assertions.

This is a convenience wrapper for writing golden file tests that handles iterating over golden files and any GOLDEN-TEST-CASEs they include, as well as reporting status about the current file and test case being processed.

This aids debuggability in case of failing tests.

stylewarning · 2019-06-04T21:30:48Z

@appleby I hate to do it, but do you mind if i squash?

appleby · 2019-06-04T21:32:54Z

@appleby I hate to do it, but do you mind if i squash?

Please do. I only switched over from force-pushing to micro commits because I assumed everyone was tired of staring at giant mutating diff.

This test was moved into printer-tests.lisp and renamed to TEST-INSTRUCTION-FMT in quil-lang#257. It also got a similar refactoring (without moving files) in quil-lang#263, and hence wound up in both places, presumably in a merge-conflict gone awry.

This test was moved into printer-tests.lisp and renamed to TEST-INSTRUCTION-FMT in #257. It also got a similar refactoring (without moving files) in #263, and hence wound up in both places, presumably in a merge-conflict gone awry.

* Fix printing of EXPT in delayed expressions Ensure that the lisp function EXPT maps to the Quil infix operator "^" when printing a DELAYED-EXPRESSION in PRINT-INSTRUCTION-GENERIC. Fixes quil-lang#249 * Add more PRINT-INSTRUCTION tests + support for testing golden files - Move existing PRINT-INSTRUCTION tests into a new file tests/printer-tests.lisp - Add support for testing "golden files", which are tests files that contain a sequence of input/expected-output pairs. See the comment in tests/utilities.lisp for more info. - Add a bunch of new PRINT-INSTRUCTION tests using the new golden file format. * Rename test-print-instruction to test-instruction-fmt Rename the test and replace all (with-output-to-string (s) (cl-quil::print-instruction ... s)) with (format nil "~/cl-quil:instruction-fmt/" ...). Updates quil-lang#116 * Add hack to reset CL-QUIL::**COMMENTS** for testing purposes Resetting the **COMMENTS** table might be required for any test that wants to validate the output of PRINT-PARSED-PROGRAM for programs that contain HALT, NOP, RESET, or other instructions that are represented by singleton classes. This is because the **COMMENTS** hash table is keyed on object identity, so if any prior tests have attached rewiring comments to any singleton instructions, they will persist and be printed by PRINT-PARSED-PROGRAM. See also "Curio No. 2: Comment sharing" from quil-lang#245 * Add abstraction around quil<-->lisp function/operator conversion The purpose of this commit is to add single source of truth + some abstraction for how Quil functions and operators map to a lisp symbols and vice versa. The motivation for this change is that previously there were a handful of places were the logic for validating and/or translating between Quil and lisp was duplicated, which led to bugs when the list of valid functions, say, was updated in one place but not the other. * Move generic deftypes and type predicates into a new types.lisp file * Give credit where credit is due Mention that printer-test-files/decomposed-ccnot.quil was stolen from Nielsen & Chuang. * Add mref test to printer-test-files/delayed-expressions.quil * Add tests/printer-test-files/pragmas.quil * Improvements to PARSE-AND-PRINT-QUIL-TO-STRING Allow the caller of PARSE-AND-PRINT-QUIL-TO-STRING to specify the :PARSER and :PRINTER they want, and make sure to use this function everywhere applicable in printer-tests.lisp. * More printer-tests tweaks - Allow a golden test case to disable the fixed-point check for a particular output by leaving a comment in the corresponding Input section. - Add a golden test file defgates.quil that contains an example of an input/output pair for which the fixed-point check needs to be disabled. - Move one of the test cases that previously appeared in TEST-DEFGATE-PRINTING into delayed-expressions.quil. - Add comments explaining why the remaining test cases in TEST-DEFGATE-PRINTING and TEST-CIRCUIT-AND-DECLARE-PRINTING are not simply included in the golden parse/print tests. - Update TEST-DEFGATE-PRINTING with a NOT-SIGNALS fiasco assertion. * Add a DEFGATE ... AS PERMUTATION test to the golden printer tests * Add TEST-JUMP-TO-INTEGER-LABEL-PRINTING * Track originating file and line number when parsing golden files - Add GOLDEN-TEST-CASE structure to represent individual input/output pairs parsed from a golden test file. This structure also tracks the originating file name and line number for each test, as well as the input and output sections. - PARSE-GOLDEN-FILE{,-STREAM} now returns a list of GOLDEN-TEST-CASE structures. - TEST-PRINT-PARSED-PROGRAM-GOLDEN-FILES now includes the file name and line number of the input/output pair that caused a failure for any failing assertions. * Add MAP-GOLDEN-FILES-AND-TEST-CASES This is a convenience wrapper for writing golden file tests that handles iterating over golden files and any GOLDEN-TEST-CASEs they include, as well as reporting status about the current file and test case being processed. * Add descriptive names for DEFGATE/DEFCIRCUIT golden tests This aids debuggability in case of failing tests. * Prefix comments with ;;; not ;; inside eval-when * Convert remaining alexandria: references to spiffy new nickname a:

appleby requested a review from a team as a code owner May 24, 2019 15:26

appleby commented May 24, 2019

View reviewed changes

stylewarning reviewed May 24, 2019

View reviewed changes

stylewarning requested changes May 24, 2019

View reviewed changes

appleby force-pushed the appleby-fix-delayed-expr-printing branch 2 times, most recently from 42b7e60 to 2f64cc4 Compare May 27, 2019 17:09

appleby changed the title ~~[wip] Fix EXPT printing~~ [DO NOT REVIEW UNLESS BORED] Fix EXPT printing May 27, 2019

stylewarning reviewed May 28, 2019

View reviewed changes

appleby force-pushed the appleby-fix-delayed-expr-printing branch from 2f64cc4 to a456d01 Compare May 28, 2019 20:10

appleby commented May 28, 2019

View reviewed changes

appleby force-pushed the appleby-fix-delayed-expr-printing branch 2 times, most recently from 132a9d2 to 295b7b7 Compare May 29, 2019 14:25

appleby commented May 29, 2019

View reviewed changes

appleby added 18 commits June 4, 2019 15:03

Fix printing of EXPT in delayed expressions

f378fbb

Ensure that the lisp function EXPT maps to the Quil infix operator "^" when printing a DELAYED-EXPRESSION in PRINT-INSTRUCTION-GENERIC. Fixes quil-lang#249

Rename test-print-instruction to test-instruction-fmt

01dd974

Rename the test and replace all (with-output-to-string (s) (cl-quil::print-instruction ... s)) with (format nil "~/cl-quil:instruction-fmt/" ...). Updates quil-lang#116

Move generic deftypes and type predicates into a new types.lisp file

bb9999c

Give credit where credit is due

1dbf7c3

Mention that printer-test-files/decomposed-ccnot.quil was stolen from Nielsen & Chuang.

Add mref test to printer-test-files/delayed-expressions.quil

3d5c907

Add tests/printer-test-files/pragmas.quil

1d4f707

Improvements to PARSE-AND-PRINT-QUIL-TO-STRING

318461a

Allow the caller of PARSE-AND-PRINT-QUIL-TO-STRING to specify the :PARSER and :PRINTER they want, and make sure to use this function everywhere applicable in printer-tests.lisp.

Add a DEFGATE ... AS PERMUTATION test to the golden printer tests

56fe7f9

Add TEST-JUMP-TO-INTEGER-LABEL-PRINTING

df5bc5e

Add MAP-GOLDEN-FILES-AND-TEST-CASES

c3f6424

This is a convenience wrapper for writing golden file tests that handles iterating over golden files and any GOLDEN-TEST-CASEs they include, as well as reporting status about the current file and test case being processed.

Add descriptive names for DEFGATE/DEFCIRCUIT golden tests

b168c4c

This aids debuggability in case of failing tests.

Prefix comments with ;;; not ;; inside eval-when

de79d15

Convert remaining alexandria: references to spiffy new nickname a:

2fcde9c

appleby force-pushed the appleby-fix-delayed-expr-printing branch from 8bd1273 to 2fcde9c Compare June 4, 2019 21:05

stylewarning approved these changes Jun 4, 2019

View reviewed changes

stylewarning merged commit e1d1333 into quil-lang:master Jun 4, 2019

appleby mentioned this pull request Jun 13, 2019

Remove duplicate TEST-PRINT-INSTRUCTION #286

Merged

notmgsk mentioned this pull request Jul 2, 2019

PRINT-INSTRUCTION-GENERIC fails to print gate applications with complex-valued parameters #313

Closed

appleby mentioned this pull request Jul 3, 2019

pi/2 is pretty printed but not 2*pi #314

Closed

	IF-EXISTS has the standard common lisp meaning. See http://l1sp.org/cl/open."
	IF-EXISTS has the standard Common Lisp meaning. See http://l1sp.org/cl/open."

		RX((%arg1-%arg2)) p


		RX(2.141592653589793) 0

Fix EXPT printing #257

Fix EXPT printing #257

Conversation

appleby commented May 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notmgsk May 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

appleby commented May 24, 2019

notmgsk commented May 24, 2019

notmgsk commented May 24, 2019

appleby commented May 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notmgsk commented May 24, 2019 • edited Loading

appleby commented May 24, 2019

notmgsk commented May 24, 2019

ecpeterson commented May 24, 2019

stylewarning commented May 24, 2019

stylewarning May 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

appleby commented May 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecpeterson May 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

appleby commented Jun 4, 2019

appleby commented May 24, 2019 •

edited

Loading

notmgsk May 24, 2019 •

edited

Loading

notmgsk commented May 24, 2019 •

edited

Loading

stylewarning May 24, 2019 •

edited

Loading

ecpeterson May 31, 2019 •

edited

Loading