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

adding doctest #487

Merged
merged 20 commits into from
Oct 13, 2021
Merged

adding doctest #487

merged 20 commits into from
Oct 13, 2021

Conversation

andrea-pasquale
Copy link
Contributor

This PR aim at closing #483.
In this first commit I modified only docstrings in src/qibo/abstractions/ as a first attempt. In order to test whether the code examples in the docstrings are working every python file must be executed using the -m doctest option. I tested the modified files on my local computer and the test need to be added to qibo.
Let me know if this is ok.
If so, I can start modifying the other docstrings.

@codecov
Copy link

codecov bot commented Oct 8, 2021

Codecov Report

Merging #487 (4b2bec3) into master (e7eb84d) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #487   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           85        85           
  Lines        12084     12084           
=========================================
  Hits         12084     12084           
Flag Coverage Δ
unittests 100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/qibo/abstractions/abstract_gates.py 100.00% <ø> (ø)
src/qibo/abstractions/callbacks.py 100.00% <ø> (ø)
src/qibo/abstractions/circuit.py 100.00% <ø> (ø)
src/qibo/abstractions/gates.py 100.00% <ø> (ø)
src/qibo/core/circuit.py 100.00% <ø> (ø)
src/qibo/core/distcircuit.py 100.00% <ø> (ø)
src/qibo/core/terms.py 100.00% <ø> (ø)
src/qibo/hamiltonians.py 100.00% <ø> (ø)
src/qibo/models/circuit.py 100.00% <ø> (ø)
src/qibo/models/evolution.py 100.00% <ø> (ø)
... and 5 more

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 e7eb84d...4b2bec3. Read the comment docs.

@scarrazza
Copy link
Member

@andrea-pasquale thanks, I have two questions:

  • does this changes in the docstring works well with sphinx?
  • we use pytest for all qibo tests, is it possible to integrate the approach presented with pytest?

@andrea-pasquale
Copy link
Contributor Author

  • does this changes in the docstring works well with sphinx?

I need to check, I believe that @mlazzarin was looking at something similar in #483.

  • we use pytest for all qibo tests, is it possible to integrate the approach presented with pytest?

I will find if there a simple way to do it using pytest.

@scarrazza
Copy link
Member

@andrea-pasquale ok, please go ahead.

@mlazzarin
Copy link
Contributor

I need to check, I believe that @mlazzarin was looking at something similar in #483.

My main worry is that a reader of the documentation should be able to copy-paste the code without manually removing the >>>.
I tried on my laptop and it seems like the >>> symbols are excluded from the selection, so it works, (even if empty lines are added between the lines of the pasted code, I don't know why).

Doctest

By the way, I was just saying that there is a Sphinx extension that I believe does something similar to doctest, but I didn't look into it. However, it may be useful if doctest does not fit well with our implementation.

@andrea-pasquale
Copy link
Contributor Author

I changed my approach and I decided to apply the sphinx extension.
Therefore now all the changes work well with sphinx.
To test all the docstrings it is sufficient to type make doctest before make html, so there is no need to integrate this approach with pytest.
This approach also removed all the >>> which may have been annoying.

There are a couple of issues:

  • the print output are commented in the case of tensors
  • the docstrings tested are only those one which appear in the documentation

@andrea-pasquale andrea-pasquale changed the title first doctest implementation adding doctest Oct 9, 2021
@andrea-pasquale andrea-pasquale changed the title adding doctest [WIP] adding doctest Oct 9, 2021
@andrea-pasquale
Copy link
Contributor Author

I updated also the docstrings in the documentation.
The only thing left is the printed tensors, I temporarily commented all those lines.
I looked for a solution to skip those lines in the test but I couldn't find.
There are also a few errors which I will show.

Comment on lines 827 to 829
#for _ in range(nepochs):
# optimize(params)
# DOCTEST ERROR: RuntimeError: Unable to cast Python instance to C++ type (compile in debug mode for details)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Casting error.

Copy link
Member

Choose a reason for hiding this comment

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

Modifying the loss calculation in the optimize function as follows:

final_state = c().state()
fidelity = tf.math.abs(tf.reduce_sum(tf.math.conj(target_state) * final_state))
loss = 1 - fidelity

should fix this. Also the params variable is defined twice above, you can remove one of the two definitions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the function from

@tf.function
def optimize(params):
    with tf.GradientTape() as tape:
        c = models.Circuit(2)
        c.add(gates.RX(0, theta=params[0]))
        c.add(gates.RY(1, theta=params[1]))
        fidelity = tf.math.abs(tf.reduce_sum(tf.math.conj(target_state) * c()))
        loss = 1 - fidelity
    grads = tape.gradient(loss, params)
    optimizer.apply_gradients(zip([grads], [params]))

to

@tf.function
    def optimize(params):
        with tf.GradientTape() as tape:
            c = models.Circuit(2)
            c.add(gates.RX(0, theta=params[0]))
            c.add(gates.RY(1, theta=params[1]))
            final_state = c().state()
            fidelity = tf.math.abs(tf.reduce_sum(tf.math.conj(target_state) * final_state))
            loss = 1 - fidelity
        grads = tape.gradient(loss, params)
        optimizer.apply_gradients(zip([grads], [params]))

and I still get the error. Maybe it is my machine. I can try and include this in the next commit to see if there is an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is the decorator @tf.function which causes problems with doctest. After removing it everything works fine. Let me know if you would like a different solution.

@@ -87,7 +92,7 @@ On the other hand, when using the ``tensorflow`` backend Qibo inherits
Tensorflow's defaults for CPU thread configuration.
Tensorflow allows restricting the number of threads as follows:

.. code-block:: python
.. testcode::

import tensorflow as tf
tf.config.threading.set_inter_op_parallelism_threads(1)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also get the following error here: RuntimeError: Inter op parallelism cannot be modified after initialization.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this is more tricky given that tensorflow must be imported before qibo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that we can just leave this block as a .. code-block:: python.

@@ -769,9 +824,9 @@ For example:
optimizer.apply_gradients(zip([grads], [params]))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The last error is this one: ValueError: No gradients provided for any variable: ['Variable:0'].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that there was a missing indentation in the line before. Now it works with doctest.

@andrea-pasquale andrea-pasquale marked this pull request as ready for review October 12, 2021 11:37
@andrea-pasquale andrea-pasquale changed the title [WIP] adding doctest adding doctest Oct 12, 2021
Copy link
Contributor

@mlazzarin mlazzarin left a comment

Choose a reason for hiding this comment

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

Looks good to me, great work! I left a few minor comments.

- name: Test with pylint
run: |
pip install pylint
pylint src -E -d E1123,E1120
- name: Test with pytest core
run: |
pytest --cov=qibo --cov-report=xml --pyargs qibo
- name: Test documentation examples
if: startsWith(matrix.os, 'ubuntu') && matrix.python-version == '3.9'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we testing only on Python 3.9?

Copy link
Member

Choose a reason for hiding this comment

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

I think we can unlock to all OS and python versions. The only filtering is applied to examples which can take a long time to run.

doc/source/code-examples/advancedexamples.rst Show resolved Hide resolved
doc/source/code-examples/advancedexamples.rst Outdated Show resolved Hide resolved
doc/source/code-examples/advancedexamples.rst Outdated Show resolved Hide resolved
doc/source/code-examples/advancedexamples.rst Outdated Show resolved Hide resolved
doc/source/code-examples/examples.rst Outdated Show resolved Hide resolved
doc/source/code-examples/examples.rst Outdated Show resolved Hide resolved
src/qibo/parallel.py Show resolved Hide resolved
@@ -836,7 +836,6 @@ For example:

for _ in range(nepochs):
optimize(params)

Copy link
Contributor

Choose a reason for hiding this comment

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

@andrea-pasquale Actually, I was thinking about removing only the white spaces, not the whole line, but I guess I incorrectly used the code suggestion feature of GitHub. The same applies with the other three edits in this commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I will change it now. I was just wondering why the tests failed.

Copy link
Member

Choose a reason for hiding this comment

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

That's a good question, but I am not sure if the issue is related to this PR. Try making the change and pushing again and we will see if the issue remains.

Copy link
Member

@stavros11 stavros11 left a comment

Choose a reason for hiding this comment

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

Thank you for this implementation, looks good to me too. My only comment below is about not removing @tf.function because it is relevant for this example.

optimizer = tf.keras.optimizers.Adam()
target_state = tf.ones(4, dtype=tf.complex128) / 2.0
params = tf.Variable(tf.random.uniform((2,), dtype=tf.float64))


@tf.function
Copy link
Member

Choose a reason for hiding this comment

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

We should not remove @tf.function here because this example is about using Tensorflow in compiled mode, in contrast to the previous one where we use eager mode. @tf.function is used to compile the optimization routine.

My doctest also fails when using @tf.function. However if I copy and paste this in a seperate python script and run it seperately it works. @andrea-pasquale, could you confirm this for your configuration? Just copy this code and run it as standalone script, it should work. If yes, then it is not clear to me why doctest fails. Ideally we would like to figure this out, but if no the simplest solution would be to disable this test as you did above for the tf threading. I believe it is not a big issue to disable since we know that the script works if run separately.

Copy link
Contributor Author

@andrea-pasquale andrea-pasquale Oct 12, 2021

Choose a reason for hiding this comment

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

Yes, as standalone script works for me as well, only doctest fails.
Let me just see if I can find a solution, otherwise I will disable the test.

Copy link
Member

Choose a reason for hiding this comment

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

Great, if you cannot find anything and @scarrazza agrees we can just disable the test and use @tf.function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After searching for a bit I found out that the problem is that the decorated function doesn't get the __doc__ attribute of the original function. One could try and include it in the class of the decorator, in this case since we are dealing with a third party decorator it is not trivial. A possibile solution is to write a new decorator that acts as the previous one and to include the __doc__ attribute:

def third_party_decorator(f):
    # badly behaved decorator, this removed f's docstring
    def no_docstring():
        return f()
    return no_docstring

old_third_party_decorator = third_party_decorator

def third_party_decorator(f):
    # replace third_party_decorator AND preserve docstring
    new_f = old_third_party_decorator(f)
    new_f.__doc__ = f.__doc__
    return new_f

I believe that in the documentation we should keep the tf.function decorator instead of some newly defined operator.
I will disable the test for now, if we find a better solution we can always include it later on.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for looking into this. I agree that we should keep it simple since this is user documentation after all.

Another approach that avoids the decorator at all is to define a new function compiled_optimize = tf.function(optimize) and use this in the place of optimize in the code that follows. In Tensorflow you can compile either using the decorator or by calling tf.function on the function you want to compile. If the decorator is the issue with doctest, this solution may also work. Sorry that I forgot to mention it before.

Copy link
Contributor Author

@andrea-pasquale andrea-pasquale Oct 12, 2021

Choose a reason for hiding this comment

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

Thanks for the suggestion. I tried this on my laptop:

def optimize(params):
        with tf.GradientTape() as tape:
            c = models.Circuit(2)
            c.add(gates.RX(0, theta=params[0]))
            c.add(gates.RY(1, theta=params[1]))
            final_state = c().state()
            fidelity = tf.math.abs(tf.reduce_sum(tf.math.conj(target_state) * final_state))
            loss = 1 - fidelity
            grads = tape.gradient(loss, params)
        optimizer.apply_gradients(zip([grads], [params]))

compiled_optimize = tf.function(optimize)

for _ in range(nepochs):
         compiled_optimize(params)

and I still got the previous error: RuntimeError: Unable to cast Python instance to C++ type (compile in debug mode for details). I guess no matter how we write it doctest doesn't like compiled functions.

Copy link
Member

Choose a reason for hiding this comment

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

Alright, thanks for checking. Then I'd suggest to keep the decorator and disable the test.

Copy link
Member

@scarrazza scarrazza left a comment

Choose a reason for hiding this comment

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

@andrea-pasquale thank you very much for all changes.

@scarrazza scarrazza merged commit 90bb1db into master Oct 13, 2021
@scarrazza scarrazza deleted the doctest branch November 7, 2021 22:15
@mlazzarin mlazzarin mentioned this pull request Nov 11, 2021
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.

4 participants