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

[READY]Add coverage in CI #8

Merged
merged 17 commits into from Sep 24, 2018
Merged

[READY]Add coverage in CI #8

merged 17 commits into from Sep 24, 2018

Conversation

@utensil
Copy link
Member

utensil commented Sep 21, 2018

This PR:

  • Move tests to a seperate folder test
  • adds coverage except it's using pytest and CodeCov instead of nosetests and coveralls in pygae/clifford
  • adds TravisCI and CodeCov badges: Build Status codecov
  • Remove NumPy dependency
  • Add simple install and test instructions in README
  • Validate existing Jupyter notebooks using nbval
@utensil utensil force-pushed the coverage branch 3 times, most recently from f5fb51e to 97beb1a Sep 21, 2018
@utensil utensil force-pushed the coverage branch from 97beb1a to db25b9d Sep 21, 2018
@utensil utensil self-assigned this Sep 21, 2018
@utensil utensil mentioned this pull request Sep 21, 2018
3 of 7 tasks complete
@utensil utensil requested review from arsenovic and hugohadfield Sep 21, 2018
Copy link
Collaborator

hugohadfield left a comment

Looks good 👍

@utensil

This comment has been minimized.

Copy link
Member Author

utensil commented Sep 22, 2018

@hugohadfield Sorry, after I requested the review last night, without seeing your approval, I felt there are more to be done in a PR this morning (in my timezone), so I added a few commits.....

Especially please see my changes in README and see if it's appropriate.

@utensil utensil changed the title Add coveralls Add coverage in CI Sep 22, 2018
@codecov

This comment has been minimized.

Copy link

codecov bot commented Sep 22, 2018

Codecov Report

❗️ No coverage uploaded for pull request base (master@7c664a1). Click here to learn what that means.
The diff coverage is 22.22%.

Impacted file tree graph

@@           Coverage Diff           @@
##             master     #8   +/-   ##
=======================================
  Coverage          ?    40%           
=======================================
  Files             ?      7           
  Lines             ?   4959           
  Branches          ?      0           
=======================================
  Hits              ?   1984           
  Misses            ?   2975           
  Partials          ?      0
Impacted Files Coverage Δ
galgebra/lt.py 11.07% <12.5%> (ø)
galgebra/printer.py 30.3% <16.66%> (ø)
galgebra/mv.py 37.07% <50%> (ø)
galgebra/ga.py 62.75% <50%> (ø)

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 7c664a1...32a5334. Read the comment docs.

@utensil

This comment has been minimized.

Copy link
Member Author

utensil commented Sep 22, 2018

I've changed from Coveralls to CodeCov ( I've used both in the past ) because the latter feels more modern and Coveralls seems to be quite slow.

Also I've chosen pytest over nosetests because I was using it and I saw the following on https://nose.readthedocs.io/en/latest/:

Nose has been in maintenance mode for the past several years and will likely cease without a new person/team to take over maintainership. New projects should consider using Nose2, py.test, or just plain unittest/unittest2.

and when I go to https://github.com/nose-devs/nose2 , I saw:

Even though unittest2 plugins never arrived, nose2 is still being maintained! ...... However, given the current climate, with much more interest accruing around pytest, nose2 is prioritizing bugfixes and maintenance ahead of new feature development.

@utensil

This comment has been minimized.

Copy link
Member Author

utensil commented Sep 22, 2018

Oops, I found a bug in my last PR when porting to 3, trying to fix it and add tests for it now.

@utensil utensil force-pushed the coverage branch from f46e26a to e727e9e Sep 22, 2018
@utensil utensil force-pushed the coverage branch from 66594c4 to 153114c Sep 22, 2018
@utensil utensil force-pushed the coverage branch from 153114c to 32a5334 Sep 23, 2018
@utensil utensil changed the title Add coverage in CI [READY]Add coverage in CI Sep 23, 2018
@utensil utensil requested a review from hugohadfield Sep 23, 2018
Copy link
Collaborator

hugohadfield left a comment

Skimming this, looks good! Maybe clear the output of the jupyter notebooks included?

@arsenovic

This comment has been minimized.

Copy link
Member

arsenovic commented Sep 24, 2018

@hugohadfield we should use nbval with clifford docs. maybe just its lax option, since we strip the output.
ill take a look

@hugohadfield

This comment has been minimized.

Copy link
Collaborator

hugohadfield commented Sep 24, 2018

Ok cool, I actually have very little idea how these notebook style tests work etc, I'll have a read into them as well when I get a chance!

@utensil

This comment has been minimized.

Copy link
Member Author

utensil commented Sep 24, 2018

@hugohadfield we should use nbval with clifford docs. maybe just its lax option, since we strip the output.
ill take a look

@arsenovic nbval checks both LaTeX output and text output (which is not visible in jupyter notebook), I've verified them with many local runs before I fix the notebooks and corresponding codes in galgebra itself. It's very helpful if we have many notebooks and want to make sure they are not outdated. That's also why I chose to keep the output for now.

"from sympy import symbols\n",
"from printer import Format\n",
"from galgebra.printer import Format\n",
"\n",

This comment has been minimized.

Copy link
@utensil

utensil Sep 24, 2018

Author Member

The way to import is changed now.

@hugohadfield

This comment has been minimized.

Copy link
Collaborator

hugohadfield commented Sep 24, 2018

Sounds sensible @utensil :)

dict_rep = {}
n = len(basis)
if mat_rep.rows != n or mat_rep.cols != n:
raise ValueError('Matrix and Basis dimensions not equal for Matrix = ' + str(mat))
raise ValueError('Matrix and Basis dimensions not equal for Matrix = ' + str(mat_rep))

This comment has been minimized.

Copy link
@utensil

utensil Sep 24, 2018

Author Member

This was an undefined variable, caused by a typo.

n_range = list(range(n))
for row in n_range:
dict_rep[basis[row]] = S(0)
for col in n_range:
dict_rep[basis[row]] += mat_rep[row,col]*basis[col]
dict_rep[basis[row]] += mat_rep[col,row]*basis[col]

This comment has been minimized.

Copy link
@utensil

utensil Sep 24, 2018

Author Member

The original usage of the class Transpose which calls _entry with a parameter expand, but it will call the _entry method of normal matrix class which don't have such a parameter. So I removed the usage of Transpose, instead directly change the order of indexes to get the result.

@@ -1048,7 +1048,7 @@ def proj(self, bases_lst):
return Mv(obj, ga=self.Ga)

def dual(self):
mode = ga.Ga.dual_mode_value
mode = self.Ga.dual_mode_value

This comment has been minimized.

Copy link
@utensil

utensil Sep 24, 2018

Author Member

This was a reference to ga. It won't work after the removal of circular dependency. And given the context, it actually meant self.Ga, just like in other methods. It's a hidden typo which isn't easy to be found before the removal of circular dependency on ga.

# self.terms.sort(key=operator.itemgetter(1), cmp=Pdop.compare)
# terms are in the form of (coef, pdiff)
# so we need to first extract pdiff and then use Pdop.compare to compare
self.terms.sort(key=cmp_to_key(lambda term1, term2 : Pdop.compare(term1[1], term2[1])))
return

This comment has been minimized.

Copy link
@utensil

utensil Sep 24, 2018

Author Member

This is where I was wrong in last PR.

@@ -328,6 +327,9 @@ def __rmul__(self, LT):
else:
raise TypeError('Cannot have LT as left argument in Lt __rmul__\n')

def __repr__(self):
return str(self)

This comment has been minimized.

Copy link
@utensil

utensil Sep 24, 2018

Author Member

Without this, it will product the correct LaTeX output with an incorrect text output invisible in a Jupyter notebook. Found it with nbval.

@utensil

This comment has been minimized.

Copy link
Member Author

utensil commented Sep 24, 2018

Thanks for the review, now I'll merge into master. (also added a few comments on some code which fixed problems but not quite visible in the review). Further fix could be done in a separate PR.

@utensil utensil merged commit 21ed56d into master Sep 24, 2018
6 checks passed
6 checks passed
Travis CI - Branch Build Passed
Details
Travis CI - Pull Request Build Passed
Details
codecov/patch No report found to compare against
Details
codecov/project No report found to compare against
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@utensil utensil mentioned this pull request Feb 14, 2019
5 of 5 tasks complete
@utensil utensil deleted the coverage branch Feb 25, 2019
@eric-wieser

This comment has been minimized.

Copy link
Contributor

eric-wieser commented on test/test_test.py in 6793b1c Nov 14, 2019

Moving these to __init__.py doesn't work - they have to be (first) in the file that they concern, as they fundamentally change the parser.

We should re-add these lines (or drop python 2, whichever comes first)

This comment has been minimized.

Copy link
Member Author

utensil replied Nov 14, 2019

I'm under the impression that they work...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.