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

Add a workflow to check for examples in docs #459

Merged
merged 97 commits into from
Feb 13, 2024
Merged

Conversation

purva-thakre
Copy link
Collaborator

Description

Fixes #289

Todos

Notable points that this PR has either accomplished or will accomplish.

  • TODO 1

Questions

  • Question1

Status

  • Ready to go

@purva-thakre
Copy link
Collaborator Author

purva-thakre commented Feb 2, 2024

To do: If I locally run make linkcheck or make doctest will add files c632cef

We don't want to add these files.

Copy link

codecov bot commented Feb 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e0e89f9) 98.1% compared to head (3be51d8) 98.1%.

Additional details and impacted files
@@          Coverage Diff           @@
##           master    #459   +/-   ##
======================================
  Coverage    98.1%   98.1%           
======================================
  Files         160     160           
  Lines        3107    3108    +1     
  Branches      749     749           
======================================
+ Hits         3048    3049    +1     
  Misses         37      37           
  Partials       22      22           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@purva-thakre
Copy link
Collaborator Author

Doctest is not correctly identifying the examples in the docstrings.

No tests are run in the added job.

image

Locally, I can see multiple failures due to float comparisons.

Step 1: We want this workflow to fail.

@purva-thakre
Copy link
Collaborator Author

purva-thakre commented Feb 7, 2024

Doctest is not correctly identifying the examples in the docstrings.

This should be fixed with 9813786. Apparently, make html has to be used before make doctest.

All the examples are correctly identified now. The issue that I am running into is that the Github Actions environment can't find any of the installed packages.

https://stackoverflow.com/a/75711927/10241324

@purva-thakre
Copy link
Collaborator Author

purva-thakre commented Feb 8, 2024

Fixed the import issue. Yay!

If poetry is used to install a package, have to make sure poetry run is used before the command we want to run. This is because poetry installs things in a virtual environment.

image

@purva-thakre
Copy link
Collaborator Author

purva-thakre commented Feb 8, 2024

@vprusso heads up! This PR is going to have a huge diff. I can't run make doctest for 1 folder. Here's a link to all that's failing: https://github.com/vprusso/toqito/actions/runs/7824351650/job/21346779560?pr=459#step:5:2040

Do you want me to ping you every time I finish up with 1 module?

It will be a bit of a tedious process. With my 2 commits below, you can see the number of failing tests have been reduced compared to the screenshot in my other comment.

image

I do not know why the number of tests also decreased unless doctest counts a syntax failure as a failing test as well. fa09a66 had 2 such failing lines.

image

@purva-thakre
Copy link
Collaborator Author

purva-thakre commented Feb 8, 2024

This comment has been fixed with a workaround in aaf0f9d

There are other options linked in this comment if the chosen workaround is not ideal.

To do: figure out a way to declare a global approximation for all float outputs.

image

use regex? https://documenter.juliadocs.org/stable/man/doctests/#Filtering-Doctests
https://stackoverflow.com/a/2428747/10241324

Or use available doctest options: https://github.com/scientific-python/pytest-doctestplus?tab=readme-ov-file#floating-point-comparison

https://docs.python.org/3/library/doctest.html#option-flags

purva-thakre added a commit that referenced this pull request Feb 8, 2024
Comment on lines -52 to +54
>>> max_mixed(2, is_sparse=True)
<2x2 sparse matrix of type '<class 'numpy.float64'>'
>>> max_mixed(2, is_sparse=True) # doctest: +ELLIPSIS, +NORMALIZE_WHITESPACE
<2x2 sparse matrix of type '<class 'numpy.float64'>'
Copy link
Collaborator Author

@purva-thakre purva-thakre Feb 8, 2024

Choose a reason for hiding this comment

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

I had to use # doctest: +ELLIPSIS, +NORMALIZE_WHITESPACE here because I couldn't figure out what the difference between the expected output vs. what doctest got was even after I made sure the indentation for both was the same.

https://docs.python.org/3/library/doctest.html#option-flags

image

Copy link
Owner

Choose a reason for hiding this comment

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

Hmm. I see. Wonder if this will not be an issue when/if we switch from matrix to np.array objects universally.

Copy link
Collaborator Author

@purva-thakre purva-thakre Feb 10, 2024

Choose a reason for hiding this comment

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

Wonder if this will not be an issue when/if we switch from matrix to np.array objects universally.

Don't know.

Although doctest was being very particular about the whitespace. Maybe 1 is supposed to be a tab while the other is not. ruff did not like it when I used tab to create the whitespace.

purva-thakre added a commit that referenced this pull request Feb 9, 2024
@purva-thakre purva-thakre marked this pull request as ready for review February 9, 2024 01:04
@purva-thakre
Copy link
Collaborator Author

purva-thakre commented Feb 9, 2024

@vprusso This PR is ready for a review. Feel free to take your time.

Except for some of the outstanding comments in this PR, there were some last-minute failures that I chose to skip for now. 6d8ddf1 , 5add9a0

I also use SKIP, ELLIPSIS, NORMALIZE_WHITESPACE for the examples that fail no matter what I try. The expected output vs. provided output were virtually identical. 🤷🏾‍♀️ One of my comments links the option flags allowed by doctest.

Copy link
Owner

@vprusso vprusso left a comment

Choose a reason for hiding this comment

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

Few minor comments, but overall, looks good!

@@ -308,8 +308,8 @@ def quantum_value_lower_bound(
... pred_mat[a_alice, b_bob, x_alice, y_bob] = 1
>>>
>>> chsh = NonlocalGame(prob_mat, pred_mat)
>>> chsh.quantum_value_lower_bound()
0.8535533840915605
>>> chsh.quantum_value_lower_bound() # doctest: +SKIP
Copy link
Owner

Choose a reason for hiding this comment

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

Should we have the skip here or just truncate the value?

Copy link
Collaborator Author

@purva-thakre purva-thakre Feb 10, 2024

Choose a reason for hiding this comment

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

The very first thing I tried was truncating the value. Unfortunately, this function was calculating two different values: 0.75 or 0.85 whenever make doctest was run multiple times.

Easier option was to make doctest skip this.

@@ -39,7 +39,7 @@ def is_unextendible_product_basis(vecs: list[np.ndarray], dims: list[int]) -> tu
>>>
>>> non_upb_tiles = np.array([tile(i) for i in range(4)])
>>> dims = np.array([3, 3])
>>> is_unextendible_product_basis(non_upb_tiles, dims)
>>> is_unextendible_product_basis(non_upb_tiles, dims) # doctest: +ELLIPSIS, +NORMALIZE_WHITESPACE
Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, are these comments needed here (i.e. the doctest stuff)

Copy link
Collaborator Author

@purva-thakre purva-thakre Feb 10, 2024

Choose a reason for hiding this comment

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

Yes, otherwise the test would fail even when I made sure I copied & pasted the expected output. Plus, in this particular example, the output also contains float values which already causes issues with doctest.

That was my issue with these scenarios as well. How to make it clear to a user that they don't have to use the doctest stuff?

@@ -21,12 +21,12 @@ def bb84() -> np.ndarray:

>>> from toqito.states import bb84
>>> x = bb84()
>>> print(f"|0> = {x[0][0].T}, \n |1> = {x[0][1].T}")
>>> print(f"|+> = {x[1][0].T}, \n |-> = {x[1][1].T}")
>>> print(f"|0> = {x[0][0].T}, \n |1> = {x[0][1].T}") # doctest: +ELLIPSIS, +NORMALIZE_WHITESPACE
Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, same here. I'm wondering if we just need to change something to remove this ignore we're doing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We are not ignoring this output. ELLIPSIS will make sure doctest is satisfied with a substring match and NORMALIZE_WHITESPACE will make sure doctest doesn't fail the test when the whitespace in the expected and calculated outputs is not identical.

image

image

Comment on lines -52 to +54
>>> max_mixed(2, is_sparse=True)
<2x2 sparse matrix of type '<class 'numpy.float64'>'
>>> max_mixed(2, is_sparse=True) # doctest: +ELLIPSIS, +NORMALIZE_WHITESPACE
<2x2 sparse matrix of type '<class 'numpy.float64'>'
Copy link
Owner

Choose a reason for hiding this comment

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

Hmm. I see. Wonder if this will not be an issue when/if we switch from matrix to np.array objects universally.

@purva-thakre
Copy link
Collaborator Author

purva-thakre commented Feb 11, 2024

Why are the following tests failing at random? Locally, if I see these 3 failures, I will run make doctest again and again until the workflow passes.

rebasing this branch does not help either.

The job did pass for

  1. python 3.11 but not for 3.10,
  2. reran the workflow for failed jobs, passed for 3.10 but failed for 3.12
  3. reran the workflow for failed jobs, finally passed for 3.12.

image

image

image

[-0. -0.j 0. +0.j -0. -0.j -0. -0.j]
[ 0. -0.j -0. +0.j 0. +0.j 0. -0.j]
[ 0.5+0.j -0. +0.j 0. +0.j 0.5+0.j]]
>>> np.around(measurements[0], decimals=5) # doctest: +SKIP
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Had to add this SKIP here because the py312 job kept failing due to below:

image

@purva-thakre purva-thakre merged commit bfeae4f into master Feb 13, 2024
18 checks passed
@purva-thakre purva-thakre deleted the doctest_examples branch February 13, 2024 23:21
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.

Automate examples in Docs
2 participants