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

Run doctest in CI and fix errors in the documentation examples #1193

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

LegrandNico
Copy link
Contributor

@LegrandNico LegrandNico commented Sep 18, 2022

This PR will contribute to #1191 by:

  • Creating a new action that runs doctest in CI (not working for now).
  • Fix the errors already present in the documentation (55 / 145 error remains).

The remaining errors are less trivial than formatting issues and will probably require more careful editing.

I also have trouble fixing errors where the structure of a graph is returned, pasting the actual output is not enough here, this is probably due to whitespace formatting (see the NORMALIZE_WHITESPACE option from doctest?).

@LegrandNico LegrandNico changed the title Run doctest in CI and fix errros Run doctest in CI and fix errors in the documentation examples Sep 18, 2022
@codecov
Copy link

codecov bot commented Sep 18, 2022

Codecov Report

Merging #1193 (dd22ff2) into main (69c1044) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##             main    #1193    +/-   ##
========================================
  Coverage   79.13%   79.13%            
========================================
  Files         173      173            
  Lines       48492    48492            
  Branches    10966    10309   -657     
========================================
  Hits        38374    38374            
  Misses       7625     7625            
  Partials     2493     2493            
Impacted Files Coverage Δ
aesara/gradient.py 77.16% <ø> (ø)
aesara/graph/basic.py 89.00% <ø> (ø)
aesara/tensor/rewriting/math.py 87.28% <ø> (ø)

@brandonwillard brandonwillard added documentation Improvements or additions to documentation testing refactor This issue involves refactoring enhancement New feature or request labels Sep 19, 2022
Comment on lines -403 to +406
{~_2: <aesara.scalar.basic.Add at 0x7f54dfa5a350>, ~_3: e(x, y)}
{~_2: <aesara.scalar.basic.Add object at 0x7fc277b841c0>, ~_3: ExpressionTuple((x, y))}
>>> s = unify(cons(op_lv, args_lv), add(x, y, z))
>>> s
{~_2: <aesara.scalar.basic.Add at 0x7f54dfa5a350>, ~_3: e(x, y, z)}
{~_2: <aesara.scalar.basic.Add object at 0x7fc277b841c0>, ~_3: ExpressionTuple((x, y, z))}
Copy link
Member

Choose a reason for hiding this comment

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

Looks like we need to take another approach altogether here (e.g. one that doesn't print local locations/addresses).

Comment on lines +5 to +10
branches:
- main
- checks
pull_request:
branches:
- main
Copy link
Member

Choose a reason for hiding this comment

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

Are these settings preventing the workflow from running in this PR?

@rlouf
Copy link
Member

rlouf commented Nov 24, 2022

Is there anything outstanding here @LegrandNico, besides the conflicts?

@LegrandNico
Copy link
Contributor Author

I can work on it a bit later today. I had difficulties creating a new action and I was not sure if this should be a part of the test.yml or a new file.

@rlouf
Copy link
Member

rlouf commented Nov 24, 2022

I can work on it a bit later today. I had difficulties creating a new action and I was not sure if this should be a part of the test.yml or a new file.

Great! I think a doctests.yml file is fine for now, let's focus on getting the doc tests running in CI in this PR and we can always move them later. Once this is running it will be easier to keep track of the remaining errors.

Copy link
Member

@brandonwillard brandonwillard left a comment

Choose a reason for hiding this comment

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

Looks like there are quite a few merge fixes needed now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request refactor This issue involves refactoring testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants