-
Notifications
You must be signed in to change notification settings - Fork 50
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
Symbolic Shaped
#867
Symbolic Shaped
#867
Conversation
10fe6aa
to
7ff633e
Compare
@mpharrigan @tanujkhattar could you take a look at this? |
class Shaped: | ||
shape: tuple[SymbolicInt, ...] = field(validator=validators.instance_of(tuple)) |
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.
Could you write a motivating docstring? This is a qualtran-specific concept that needs to be thoroughly documented: what it is, why we need it, how you might use it
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 attempted to explain it a bit, could you check how it looks?
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.
very cool. Some polish things in the comments
qualtran/bloqs/hamiltonian_simulation/hamiltonian_simulation_by_gqsp.ipynb
Outdated
Show resolved
Hide resolved
7dd5aa0
to
2342b88
Compare
fix expression for Jacobi-Anger degree add test using 1d ising walk op, fix call graph fix test fix hamsim nb `slen`, docstrings, generalize `is_symbolic` (to remove dep. on cirq) test shape with symbolic entries docstring refs: remove extra indent skip serialize test for symbolic hamsim
2342b88
to
17d6fa7
Compare
ca44e0b
to
653ae04
Compare
@@ -180,18 +172,6 @@ def build_composite_bloq(self, bb: 'BloqBuilder', **soqs: 'SoquetT') -> Dict[str | |||
|
|||
return soqs | |||
|
|||
def build_call_graph(self, ssa: 'SympySymbolAllocator') -> Set['BloqCountT']: |
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.
what happened to this?
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.
We don't need a specialization anymore, it works automatically through GeneralizedQSP
's call graph.
Before this, there was no way to build GQSP without the exact polynomial, so I had to override it here.
if len(args) != 1: | ||
return any(is_symbolic(arg) for arg in args) | ||
|
||
(arg,) = args |
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.
handle len(args)==0
?
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.
to my surprise, it seems any(...)
for an empty generator returns False
, so line 56 automatically handles this case
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.
ah got it, thanks
return any(is_symbolic(arg) for arg in args) | ||
|
||
(arg,) = args | ||
if isinstance(arg, sympy.Basic): |
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 think we should be checking isinstance(arg, sympy.Expr)
, right?
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 sure actually, I just copied it from the existing code.
It seems boolean expressions are instances of sympy.Basic
but not sympy.Expr
(https://docs.sympy.org/latest/explanation/glossary.html#term-Expr), which we may want to use for symbolic controls?
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.
Thanks. Interesting reading. Happy keeping this as Basic for now
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.
Very useful contribution for more symbolic costings!
Discussion: #801