-
Notifications
You must be signed in to change notification settings - Fork 22
improving coverage with some tricks and judicious choices #176
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
Conversation
| degree, s = params[0:2] | ||
| if 'iterate' in options and options['iterate']: | ||
| num_iterations = params[2] | ||
| if options != None: |
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.
This way these lines get skipped by coverage, because they're uninteresting and will be removed eventually anyway.
| - name: tests and coverage | ||
| run: | # some extra stuff here to make sure coverage works across multiprocessing | ||
| pip install -e .[advanced,dev] coveralls | ||
| coverage run --source=pynumdiff --omit="pynumdiff/_version.py,pynumdiff/tests/*,pynumdiff/utils/old_pi_cruise_control.py" -m pytest -s |
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.
The omits can live in .coveragerc, since I need one now anyway, and they're complicated enough that they probably should live there.
| import matplotlib.pyplot as plt | ||
| except ImportError: | ||
| _has_matplotlib = False | ||
| print("Matplotlib is not installed - plotting functionality disabled") |
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 don't think it's worth having all this up here. If you try to import something but don't have it, python will give you a message about it already.
| c = np.ones(n + 1) / dx | ||
| D = sparse.spdiags([-c, c], [0, 1], n, n + 1) | ||
|
|
||
| DT = D.transpose() |
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.
Why call .transpose() when .T exists?
| def linop(v): return (alph * L * v + AT(A(v))) | ||
| linop = splin.LinearOperator((n + 1, n + 1), linop) | ||
|
|
||
| [s, info_i] = sparse.linalg.cg(linop, g, x0=None, rtol=rtol, maxiter=maxit, callback=None, M=P, atol=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.
This identical line was happening inside the if and the else. Just pull it out and obviate the need for the else.
|
|
||
| if __name__ == "__main__": | ||
|
|
||
| if __name__ == "__main__": # Small testing script |
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.
This script is now getting ignored by coverage.
Just like it's possible to skip files and not count them in coverage, it's possible to skip functions or blocks headed by lines that match certain regex. I've added a bunch of rules to skip things like plotting code, method stubs left over for those that moved submodules, some of the
params/options-related variable assignments that are purely for backwards compatibility and trivial but happen to be uncovered in tests, ImportError-related warnings mostly concerned withcvxpy, a__main__demo script, and an alternative way of simulating the lorenz system that we happen not to be calling but is short and kinda neat (enough to keep). I've also decided to skipsuggest_methodeven though I think it should be tested, because it takes a long time to run so isn't great for unit tests that get kicked off by CI runs all the time. Rather, it's covered by notebook 5 like plotting code is covered by notebooks 1 and 2. Encoding these directives is best done in a.coveragerc, so I just put all theomitfilenames in there too to simplify thecoverage runflags intest.yml.