-
Notifications
You must be signed in to change notification settings - Fork 68
0.4.3 release #11
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
0.4.3 release #11
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11 +/- ##
==========================================
+ Coverage 40% 48.89% +8.89%
==========================================
Files 7 8 +1
Lines 4959 4947 -12
==========================================
+ Hits 1984 2419 +435
+ Misses 2975 2528 -447
Continue to review full report at Codecov.
|
@hugohadfield @arsenovic The release of 0.4.3 is ready for review, thanks. |
|
||
script: | ||
# Run tests, validate Jupyter notebooks and generate coverage report | ||
- pytest --cov=galgebra --nbval examples/ipython/ --nbval doc/ipython/ --nbval doc/python/ --current-env test |
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.
Tests covers more forms of tests and collects the coverage stat data in one go.
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.
Thats neat
from __future__ import absolute_import, division | ||
from __future__ import print_function | ||
from galgebra.printer import Format, xpdf | ||
from galgebra.ga import Ga |
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.
If the users install galgebra from pip, the way of importing the modules has to conform to the standard "namespaced" way instead of just importing modules from a global namespace. This might break some old code but it's a necessary transition process.
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.
agreed
u = kargs[0] # Coordinate map or vector embedding to define submanifold | ||
coords = kargs[1] # List of cordinates | ||
ga = kwargs['ga'] # base geometric algebra | ||
self.wedge_print = kwargs['wedge'] |
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 line was clearly a copy-paste bug and this self.wedge_print turns out to be not used and useless for this class.
else: | ||
for (coef, base) in zip(coefs, bases): | ||
obj += modes(coef) * base | ||
obj += Simp.modes(coef) * base |
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.
An example of using non-existent non-namespaced method.
for (coef, base) in zip(coefs, bases): | ||
obj += modes(coef) * base | ||
mv.obj = obj | ||
mv.obj = Simp.apply(mv.obj) |
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 code was repetitive, so calls the method with exactly the same semantic instead.
g_lst = [] | ||
g_inv_lst = [] | ||
for coord1 in self.coords: | ||
i1 = str(coord2) |
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.
Incorrect local variable naming, probably just a copy-paste bug. It was not covered in previous tests so it was not detected. Now it's covered and passed.
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.
great, its really nice the tests are running well
return A > B | ||
else: | ||
raise ValeError('Operation ' + op + 'not allowed in Mv.Mul!') | ||
raise ValueError('Operation ' + op + 'not allowed in Mv.Mul!') |
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.
A typo bug.
elif isinstance(sdop, tuple): | ||
self.term.append(sdop) | ||
self = Dfop.consolidate_coefs(self) | ||
self = Sdop.consolidate_coefs(self) |
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.
Use the correct version of consolidate_coefs method.
|
||
if dop1.cmpflg != dop2.cmpflg: | ||
raise ValueError('In Dop.Add complement flags have different values.') | ||
raise ValueError('In Dop.Add complement flags have different values: %s vs. %s' % (dop1.cmpflg, dop2.cmpflg)) |
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.
cmpflg still has some potential bugs. This is to make the exception to have proper context.
else: | ||
if self.cmpflg: | ||
s += str_dop + '*' + str_base | ||
s += str_sdop + '*' + str_base |
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.
Also a local variable name typo
def com(A, B): | ||
return ga.Ga.com(A, B) | ||
raise ImportError( | ||
"""mv.com is removed, please use ga.Ga.com(A, B) instead.""") |
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.
Added a deprecation error.
\\DeclareMathOperator{\Adj}{Adj} | ||
""" | ||
$\\DeclareMathOperator{\\Tr}{Tr} | ||
\\DeclareMathOperator{\\Adj}{Adj} |
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.
Many backslashes are not properly escaped, caused some warnings and now fixed.
from distutils.core import Extension | ||
|
||
VERSION = '0.4.1.1' | ||
VERSION = '0.4.3' |
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 version bump.
'Natural Language :: English', | ||
'Operating System :: OS Independent', | ||
'Programming Language :: Python :: 2.7', | ||
'Programming Language :: Python :: 3', |
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.
Py3 support. 🎆
Get compatible with python 3 | ||
|
||
Get Travis set up with some good tests | ||
|
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.
These 2 TODOs are done. 😄
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.
It looks like a load of nice work bringing this library back to life
@utensil what is your pypi username? I think it makes sense to add you as an owner on https://pypi.org/manage/project/galgebra/ |
It's https://pypi.org/user/utensil/ , thanks~ |
@hugohadfield Thanks for the review, I'll merge now, would you please release it to the PyPI? |
This is a work-in-progress for the release of 0.4.3 which is discussed in #10 and https://github.com/brombo/galgebra/pull/42#issuecomment-462263950 .
This PR includes the following: