Skip to content

Allow vector patterns to be used in kernels - #38

Open
croyzor wants to merge 3 commits into
mainfrom
kernel-vec-patterns
Open

Allow vector patterns to be used in kernels#38
croyzor wants to merge 3 commits into
mainfrom
kernel-vec-patterns

Conversation

@croyzor

@croyzor croyzor commented Oct 14, 2024

Copy link
Copy Markdown
Collaborator
  • Add our vector patterns to the map of allowed kernel constructors (for kernel Vec)
  • Add an example file examples/brick.brat
  • Update detectVecErrors to deal with the existence of more complicated vector tests than "0 or succ?". This is very much a bandage over a system that needs to be completely redone, but I hit up against this trying to write the example and it's errors should be slightly more informative now

@croyzor
croyzor requested a review from acl-cqc October 14, 2024 15:14
Comment thread brat/Brat/Checker.hs
-- We only want to know if the vector length is nil or a successor
toLenConstr :: NumPat -> Either NumPat (LengthConstraint)
toLenConstr NP0 = pure $ Length 0
toLenConstr (NP1Plus (NP2Times np)) = either (const (Right LengthOdd)) Right (toLenConstr np)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok...so....what does this do.....

  • If toLenConstr np returns (Left error), we discard the error and return pure LengthOdd
  • If toLenConstr np returns a Right value, we leave it unchanged

Is that right? (This is quite hard to follow)

If so, how about pure $ fromRight LengthOdd (toLenConstr np) ?

Comment thread brat/Brat/Checker.hs
err $ getVecErr tp (show ty) (show n) p'
Left (NumMatchFail _ _) -> case (toLenConstr p) of
Right p' -> err $ getVecErr tp (show ty) (show n) p'
Left p' -> err $ InternalError ("detectVecErrors: Unexpected pattern: " ++ show (toLenConstr p'))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's this toLenConstr p'? This seems odd. So after toLenConstr has stripped away any outer (NP1Plus?) NPTwoTimess from the input p to give p', we then call it again to construct an error message? Is that right?

I'm hoping this should/could just be show p' in which case I have a different suggestion - how about moving the error-construction into toLenConstr (so it's :: NumPat -> Either Error LengthConstraint) and then using throwLeft here?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, it's a bit hard to follow as is

Comment thread brat/Brat/Checker.hs
toLenConstr :: NumPat -> Checking LengthConstraint
-- Try to work out the length of a vector
-- We only want to know if the vector length is nil or a successor
toLenConstr :: NumPat -> Either NumPat (LengthConstraint)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Superfluous extra brackets here, at least

Comment thread brat/examples/brick.brat
@@ -0,0 +1,20 @@
-- Create a "brickwork" state
-- Apply a parameterised unitary U to entangle every adjacent pair of qubits in a line architecture.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eeek - skipping dependent types to ensure weights match in length, is this roughly just
brick(th ,- ths, U) = {q0 ,- q1 ,- qrest => let (q0,q1) = U(th)(q0,q1) in q0 ,- brick(ths, U)(q1 ,- rest)? (I.e. gate the first two qubits, then proceed "down the line" - what I thought was called a "ladder" architecture)?

If so then whilst a fun bit of list comprehensions they don't really do much useful (you're just riffling things apart and back together, and so on), right?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes you're right, it's just a bit of fun that tests vector patterns are working in kernels 🙂

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Terminology-wise, I think this is called a brickwork pattern and a ladder is when everything else is sequentially entangled with 1 qubit

Comment thread brat/Brat/Constructors.hs
])
,(CConcatEqEven, M.fromList
[(CVec, CArgs [VPVar, VPNum (NP2Times NPVar)] (Sy (Sy Zy))
-- Star should be a TypeFor m forall m?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really sure I understand this comment...what happens if you replace the star with that TypeFor?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try and push something to clear this up

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My idea was that you could split on classical variables in the kernel, like

divConquer(n :: #, Vec({ Qubit -o Qubit }, n)) -> { Vec(Qubit, n) -o Vec(Qubit, n) }
divConquer(0, []) = { .. }
divConquer(doub(n), gs) = { qs0 =,= qs1 =>
  let gs0 =,= gs1 = gs in divConquer(n, xs0)(gs0) =,= divConquer(n, gs1)(qs1)
}

but having just tried that, there are more blockers (gs isn't in scope in a kernel context anyway!) so I'll remove the comments.
Also, it'd be nice to be able to do this, as long as the pattern match is irrefutable... I guess we should come back to it after we have checking coverage of patterns

Comment thread brat/Brat/Checker.hs
Left (NumMatchFail _ _) -> do
p' <- toLenConstr p
err $ getVecErr tp (show ty) (show n) p'
Left (NumMatchFail _ _) -> case (toLenConstr p) of

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So whilst continuing the slightly-hacked-in nature here and a better framework might be nice I don't think this is all that bad, certainly I'm not objecting to patching it in like this...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants