Skip to content
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

Generalize fourier spectrum #1681

Merged
merged 75 commits into from
Oct 6, 2021

Conversation

dwierichs
Copy link
Contributor

@dwierichs dwierichs commented Sep 23, 2021

Context:
This PR generalizes the qml.fourier.spectrum to more general circuits. It is part of the series of PRs that replace #1635.
After reconsiderations, this is not implemented with a separate ArgMap class (see #1653).

Description of the Change:
The spectrum is now computed with respect to QNode arguments, and no longer with respect to gate arguments. This allows to obtain the function structure with respect to parameters that actually are controlled directly by the user.
Classical preprocessing between QNode and gate parameters is taken into account and factored into the spectrum.
In order for the spectrum to actually describe the QNode fully, the return value needs to be an expectation value/a sum of expectation values, and the preprocessing needs to be linear.
spectrum checks whether the Jacobian is constant across num_pos+1 many points (including the original args and num_pos additional, random args). This does not 100% guarantee linearity of the processing but makes it very unlikely that non-linear processing is overlooked. The test can be deactivated by setting num_pos=0.

The arguments and elements of arguments for which the spectrum is computed are controlled by encoding_args, which is a dict with argument names as keys and a list of index tuples for the respective argument as values. The value may be replaced by an Ellipsis, in which case all elements of the argument are considered. If encoding_args is a set of argument names instead, it is interpreted like {name: ... for name in names}.
Alternatively, the arguments to compute the spectrum for can be chosen by their index in the function signature via argnum.
If neither encoding_args nor argnum are provided, those arguments of the QNode that do not have a default defined (and all their elements, if they are array-valued) are considered.

Benefits:
More streamlined usage of the spectrum functionality on the level of QNodes instead of looking into a QNode.
No gate ids have to be assigned anymore.
Preprocessing of QNode arguments into gate arguments is taken into account both in the spectrum output and to exclude a large class of circuits that do yield a Fourier series in the sense considered by this function.

Possible Drawbacks:
This is a breaking change in multiple ways: The input arguments changed, the output format changed and the gate ids are no longer required or considered.
Also, the content of the output changed because preprocessing is included in the spectrum.

Related GitHub Issues:
#1635
#1653

@dwierichs
Copy link
Contributor Author

We previously discussed that some multi-parameter gates that easily decompose into gates to which the Fourier analysis is applicable should have a flag like has_separable_parameters, so that spectrum decomposes them instead of throwing an Exception if parameters for which the spectrum was requested feed into such a gate.
I had a look at all parametric operations and I am not sure whether we would like to give U2, U3 and CRot this flag. If not, Rot would be the only operation with this flag and it might not be worth it introducing it after all.
A reason not to give the flag to the three gates above is that the parameter dependence is not such that the parameters are separated into different gates when decomposing. Instead, we just obtain some decomposed circuit that produces a Fourier series but then again, we would not have a single multi-parameter gate that has has_separable_parameters=False.
Happy to hear your thoughts on this, @glassnotes @josh146 .

@codecov
Copy link

codecov bot commented Sep 28, 2021

Codecov Report

Merging #1681 (1083cae) into master (192e810) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1681   +/-   ##
=======================================
  Coverage   99.21%   99.22%           
=======================================
  Files         203      204    +1     
  Lines       15300    15395   +95     
=======================================
+ Hits        15180    15275   +95     
  Misses        120      120           
Impacted Files Coverage Δ
pennylane/fourier/__init__.py 100.00% <100.00%> (ø)
pennylane/fourier/circuit_spectrum.py 100.00% <100.00%> (ø)
pennylane/fourier/qnode_spectrum.py 100.00% <100.00%> (ø)
pennylane/fourier/utils.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 192e810...1083cae. Read the comment docs.

@dwierichs dwierichs marked this pull request as ready for review September 28, 2021 10:40
@dwierichs
Copy link
Contributor Author

@josh Thanks for the review! I am not sure now whether you would like to make the two functions diffable or whether that is out of scope?
If we want to make the advanced_spectrum differentiable, this will add considerable amounts of code, as for example a set of torch tensors does not contain unique-valued objects but unique tensors:
Screenshot from 2021-10-04 14-00-24
This means that there will be multiple points in the function at which we would have to have interface-dependently dispatched versions of subroutines...

@glassnotes
Copy link
Contributor

I recall seeing a comment by @glassnotes regarding the function names, but I can't find it now thinking I do admit I prefer function names that are descriptive rather than 'advanced' or 'simple'.

@josh146 that comment is here. Unfortunately I still don't have any amazing suggestions for renaming.

@josh146
Copy link
Member

josh146 commented Oct 4, 2021

@glassnotes the link doesn't work for me 😆

Copy link
Contributor

@glassnotes glassnotes left a comment

Choose a reason for hiding this comment

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

@dwierichs no major comments, just left a couple comments/questions. There is still the matter of renaming the functions, though.

doc/releases/changelog-dev.md Outdated Show resolved Hide resolved
pennylane/fourier/advanced_spectrum.py Outdated Show resolved Hide resolved
pennylane/fourier/advanced_spectrum.py Outdated Show resolved Hide resolved
pennylane/fourier/advanced_spectrum.py Outdated Show resolved Hide resolved
pennylane/fourier/advanced_spectrum.py Outdated Show resolved Hide resolved
tests/fourier/test_advanced_spectrum.py Outdated Show resolved Hide resolved
tests/fourier/test_advanced_spectrum.py Outdated Show resolved Hide resolved
tests/fourier/test_fourier_utils.py Outdated Show resolved Hide resolved
tests/fourier/test_fourier_utils.py Outdated Show resolved Hide resolved
tests/fourier/test_simple_spectrum.py Outdated Show resolved Hide resolved
Copy link
Contributor

@glassnotes glassnotes left a comment

Choose a reason for hiding this comment

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

@dwierichs looks good to me, thanks for you work on this! 🎉

pennylane/fourier/qnode_spectrum.py Outdated Show resolved Hide resolved
>>> _process_ids(None, None, circuit)
(OrderedDict([('a', Ellipsis), ('b', Ellipsis), ('c', Ellipsis)]), [0, 1, 2])

Note that ``x`` does not appear here, because it has a default value defined and thus is
Copy link
Contributor

Choose a reason for hiding this comment

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

The new example and more detailed explanation are extremely helpful, thank you! 💯

Copy link
Member

Choose a reason for hiding this comment

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

this docstring is amazing 😭

pennylane/fourier/advanced_spectrum.py Outdated Show resolved Hide resolved
Co-authored-by: Olivia Di Matteo <2068515+glassnotes@users.noreply.github.com>
Copy link
Member

@josh146 josh146 left a comment

Choose a reason for hiding this comment

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

Great work @dwierichs! Left some minor documentation fixes, but apart from that good to go on my end 🙂

I think there is only other thing that need to be changed as part of the renaming (although this can be left for a separate PR):

  • Updating spectrum -> circuit_spectrum on the qml.fourier module docpage (worth also mentioning qnode_spectrum?)

doc/releases/changelog-dev.md Show resolved Hide resolved
doc/releases/changelog-dev.md Outdated Show resolved Hide resolved
pennylane/fourier/__init__.py Outdated Show resolved Hide resolved
>>> _process_ids(None, None, circuit)
(OrderedDict([('a', Ellipsis), ('b', Ellipsis), ('c', Ellipsis)]), [0, 1, 2])

Note that ``x`` does not appear here, because it has a default value defined and thus is
Copy link
Member

Choose a reason for hiding this comment

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

this docstring is amazing 😭

pennylane/fourier/qnode_spectrum.py Outdated Show resolved Hide resolved
pennylane/fourier/qnode_spectrum.py Outdated Show resolved Hide resolved
tests/fourier/test_qnode_spectrum.py Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-ready 👌 PRs which are ready for review by someone from the core team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants