SymPy Patches Tutorial¶
Talk is cheap. Show me the code.
—Linus Torvalds [1]
Introduction¶
In SymPy we encourage collaborative work.
Everyone is welcome to join and to implement new feature, fix some bug, give general advice, etc... Also, we try to discuss everything and to review each other’s work so that many eyes can see more thus raising the quality.
General discussion takes place on sympy@googlecode.com mailing list and in the issues, and the code is discussed in sympy-patches@googlecode.com mailing list.
As someone of you already know, software development is not just coding. A lot of non-coding tasks have to be done in order to produce good code. Just to mention a few: setting up infrastructure, designing, testing, documenting, assisting new developers (we are doing it here), and of course programming.
But even programming is not all about writing the code, it is about writing the code and preparing it so that the code can be included into the project.
Both producing the code and bringing it to the project are important parts of the game – without the code there is nothing to bring in, and having the code outside is a nowin for anyone.
As already said above, we review changes. This idea was borrowed from successful projects like Linux, Python, SAGE and a lot more. In short each change is first reviewed by other developers and only when it is approved the code is pushed in.
Like it takes effort to write good and clear code, reviewing other’s work needs effort too. There are good practices how to do things so that reviewing is fun for both the author and the reviewer. We try to follow them, and we’ll try to show you how to follow too.
This tutorial will guide you how to do SymPy development with Mercurial and patches.
By the way
when reviewing other’s patches you learn a lot, so why not to join as a reviewer too?
Workflow¶
Here we’ll describe the workflow using real world example where we fix some bug.
Hacking¶
Suppose I want to fix issue #755
I go there
Look at the problem description
...
Aah, it looks like printing code in sympy/printing/printer.py tries to treat expr as an instance of a new-style class. I think I know how to fix this!
Ok, going to dive into the problem.
First of all there exist two approaches to work:
- doing all the work in one repository, and
- creating new repository for each task
In practice both of them have pros and cons, but we’ll consider the second one as it does the job and is simpler.
So, let’s clone main repo, and go hacking
By the way
if you haven’t already done so, do
$ hg clone http://hg.sympy.org/sympy
$ hg clone sympy sympy-fix755 updating working directory 386 files updated, 0 files merged, 0 files removed, 0 files unresolved $ cd sympy-fix755
Note
This is a real-life example. If you want to repeat the exact steps, update your working directory to 4962f6641827 revision now:
$ hg up 4962f6641827
I use vim editor, and usually it is very handy to navigate by tags, so I build tags database first, then invoke my editor [2]
$ ctags -R $ vim
It was in _print function, so I locate it using tags
here it is
and here is the failing code
Aah, it seems old classes do not have the attribute __class__. Let’s see if this is the gist of the problem.
$ ./bin/isympy In [1]: class C: pass ...: In [2]: C ————————————————————————— exceptions.AttributeError Traceback (most recent call last) ... AttributeError: class C has no attribute ‘__class__’
Yes, it is. So I change it to what would always work. Then I invoke hq [3] to see what I’ve changed:
$ hq
and its time to see whether this fixes the problem
In [1]: import sympy In [2]: sympy.__dict__
oops, another bug seems to be sitting there:
after some examination it turned out to be a problem related to pretty-printing of sequences with even height
In [3] {x**2: 1}
—————————————————————————
exceptions.ValueError Traceback (most recent call last)
...
ValueError: xobj: expect length = 2*k+1
I decided to add another row for this case
$ hqq # full diff at this stage
since after all it looks good:
In [1]: [x**2]
Out[1]:
⎡ 2⎤
⎣x ⎦
In [2]: (x**2,)
Out[2]:
⎛ 2⎞
⎝x ⎠
In [3]: {x**2: 1}
Out[3]:
⎧ 2 ⎫
⎨x : 1⎬
⎩ ⎭
so why not?
Now let’s verify that printing tests pass
$ py.test sympy/printing/ ============================= test process starts ============================== executable: /usr/bin/python2.5 (2.5.0-final-0) using py lib: /home/kirr/src/tools/py/py-0.9.0/py <rev unknown> sympy/printing/tests/test_gtk.py[1] f sympy/printing/tests/test_latex.py[8] .......f sympy/printing/tests/test_mathml.py[6] .....f sympy/printing/tests/test_pretty.py[10] .........f sympy/printing/tests/test_pretty_unicode.py[10] .......... sympy/printing/tests/test_python.py[6] ...... ============== tests finished: 37 passed, 4 xfail in 0.30 seconds =============
Everything seems to be ok with pprinting evenly heighted sequences. Let’s get back to original problem:
In [1]: class C: pass ...: In [2]: C Out[2]: __main__.C In [3]: import sympy In [4]: sympy.__dict__ (a lot is printed)
ok, it works.
Let’s write tests (this is important)
$ hqq # 2.patch
Also, when looking at _print function again, I’ve noticed there are typos in printer.py so I’ve spellchecked the whole file
$ hqq # 3.patch
It seems we have done the job now.
To ensure everything stays in shape, let’s see if all tests pass
$ ./setup.py test running test ============================= test process starts ============================== executable: /usr/bin/python2.5 (2.5.0-final-0) using py lib: /home/kirr/src/tools/py/py-0.9.0/py <rev unknown> sympy/concrete/tests/test_gosper.py[2] .. sympy/concrete/tests/test_products.py[3] ... sympy/concrete/tests/test_sums_products.py[9] ......fff sympy/core/tests/test_arit.py[35] .................................ff sympy/core/tests/test_assumptions.py[18] ................ff sympy/core/tests/test_basic.py[24] ........................ sympy/core/tests/test_complex.py[10] .......... sympy/core/tests/test_count_ops.py[1] . sympy/core/tests/test_diff.py[3] ... sympy/core/tests/test_equal.py[4] .... sympy/core/tests/test_eval.py[8] .......f sympy/core/tests/test_eval_power.py[6] ...... sympy/core/tests/test_functions.py[20] ................fff. sympy/core/tests/test_match.py[24] .......................f sympy/core/tests/test_numbers.py[20] .................... sympy/core/tests/test_relational.py[2] .. sympy/core/tests/test_str.py[8] ........ sympy/core/tests/test_subs.py[13] ............. ... == tests finished: 712 passed, 1 xpass, 60 xfail, 4 skipped in 125.20 seconds == Testing docstrings.
Good.
Back to version control¶
We’ve finished hacking, and it’s time to remember about version control. The goal is to prepare our work for inclusion into the project.
First, there are three semantically independent changes made:
- _print dispatcher was fixed to handle old-style classes correctly
- we fixed pretty-printing to handle vertical objects of even height
- typos in sympy/printing/printer.py were fixed
It is good to structure changes, and as said in the Introduction we try to make the whole process fun for all, so let’s prepare three separate patches for each change. This way it would be easier to review them.
For this we are going to use Mercurial Queues and Record extensions.
By the way
if you haven’t already done so, include the following lines into your ~/.hgrc:
[extensions] hgext.mq = hgext.record =
I’m going to record fix for #755 first
Note
The command qrecord below was introduced in Mercurial 1.0. If you have an older version of Mercurial, consider upgrading it. If that is not possible, you can use hg qnew -f and hg qrefresh commands – they’ll do the job, although they are not-so-convenient when you work on several things at once (that’s why qrecord was invented).
$ hg qrecord –edit pprint-fix-old-classes.patch
diff –git a/sympy/printing/pretty/pretty_symbology.py b/sympy/printing/pretty/pretty_symbology.py
1 hunks, 5 lines changed
examine changes to ‘sympy/printing/pretty/pretty_symbology.py’? [Ynsfdaq?] n
diff –git a/sympy/printing/printer.py b/sympy/printing/printer.py
3 hunks, 6 lines changed
examine changes to ‘sympy/printing/printer.py’? [Ynsfdaq?] y
@@ -22,7 +22,7 @@
Also, if BAR is a subclass of FOO, _print_FOO(bar) will be called for
instance of BAR, if no _print_BAR is provided. Thus, usually, we don’t
- need to provide prining routines for every class we want to support –
+ need to provide printing routines for every class we want to support –
only generic routine has to be provided for a set of classes.
A good example for this are functions - for example PrettyPrinter only
record this change to ‘sympy/printing/printer.py’? [Ynsfdaq?] n
@@ -52,7 +52,7 @@
It’s job is to loop through expr classes (class + it’s bases), and
try to dispatch the work to _print_<EXPR_CLASS>
- e.g., suppose we have the following class hierarcy::
+ e.g., suppose we have the following class hierarchy::
Basic
|
record this change to ‘sympy/printing/printer.py’? [Ynsfdaq?] n
@@ -90,7 +90,7 @@
# See if the class of expr is known, or if one of its super
# classes is known, and use that print function
res = None
- for cls in expr.__class__.__mro__:
+ for cls in type(expr).__mro__:
if hasattr(self, ‘_print_’+cls.__name__):
res = getattr(self, ‘_print_’+cls.__name__)(expr, *args)
break
record this change to ‘sympy/printing/printer.py’? [Ynsfdaq?] y
diff –git a/sympy/printing/tests/test_pretty.py b/sympy/printing/tests/test_pretty.py
1 hunks, 9 lines changed
examine changes to ‘sympy/printing/tests/test_pretty.py’? [Ynsfdaq?] y
@@ -215,3 +215,12 @@
def test_pretty_limits():
assert pretty( limit(x, x, oo, evaluate=False) ) == ‘ lim x\nx->oo ‘
assert pretty( limit(x**2, x, 0, evaluate=False) ) == ‘ 2\nlim x \nx->0 ‘
+
+def test_pretty_class():
+ “”“test that printer dispatcher correctly handles classes”“”
+ class C: pass # C has no .__class__ and this was causing problems
+ class D(object): pass
+
+ assert pretty( C ) == “test_pretty.C”
+ assert pretty( D ) == “<class ‘test_pretty.D’>”
+
record this change to ‘sympy/printing/tests/test_pretty.py’? [Ynsfdaq?] y
diff –git a/sympy/printing/tests/test_pretty_unicode.py b/sympy/printing/tests/test_pretty_unicode.py
1 hunks, 27 lines changed
examine changes to ‘sympy/printing/tests/test_pretty_unicode.py’? [Ynsfdaq?] n
Pay attention to how I carefully choose which hunks go into this patch and which do not. Especially note how some changes made to printer.py were recorded and other changes (spelling fixes) were left intact in the work dir.
Then an editor is popped up and asks about commit message:
pprint: fix handling of old-style classes (#755) We used to dispatch based on expr.__class__.__mro__, but when expr is an old-style class we failed, because old-style classes do not have __class__ attribute. Let’s just use type(expr).__mro__ which works in all cases. HG: Enter commit message. Lines beginning with ‘HG:’ are removed. HG: – HG: user: Kirill Smelkov <kirr@landau.phys.spbu.ru> HG: branch ‘default’ HG: changed sympy/printing/printer.py HG: changed sympy/printing/tests/test_pretty.py
Now you have one patch applied
$ hg qapplied pprint-fix-old-classes.patch
and work-dir changes shrinked somewhat
$ hqq # 4.patch
note, how there is no more expr.__class__ -> type(expr) change in hqq output.
Congratulations, you’ve recorded your first patch!
Let’s proceed with pprint
$ hg qrecord -e xobj-fix-even-height.patch
diff –git a/sympy/printing/pretty/pretty_symbology.py b/sympy/printing/pretty/pretty_symbology.py
1 hunks, 5 lines changed
examine changes to ‘sympy/printing/pretty/pretty_symbology.py’? [Ynsfdaq?] y
@@ -311,7 +311,10 @@
if bot is None: bot = ext
if mid is not None:
if (length % 2) == 0:
- raise ValueError(‘xobj: expect length = 2*k+1’)
+ # even height, but we have to print it somehow anyway...
+ # XXX is it ok?
+ length += 1
+
else:
mid = ext
record this change to ‘sympy/printing/pretty/pretty_symbology.py’? [Ynsfdaq?] y
diff –git a/sympy/printing/printer.py b/sympy/printing/printer.py
2 hunks, 4 lines changed
examine changes to ‘sympy/printing/printer.py’? [Ynsfdaq?] n
diff –git a/sympy/printing/tests/test_pretty_unicode.py b/sympy/printing/tests/test_pretty_unicode.py
1 hunks, 27 lines changed
examine changes to ‘sympy/printing/tests/test_pretty_unicode.py’? [Ynsfdaq?] y
@@ -198,3 +198,30 @@
assert u == s
+def test_upretty_seq_even():
+ “”“there used to be a bug when pprinting sequences with even height”“”
+ u = upretty([x**2])
+ s = \
+u”“”\
+⎡ 2⎤
+⎣x ⎦\
+”“”
+ assert u == s
+
+ u = upretty((x**2,))
+ s = \
+u”“”\
+⎛ 2⎞
+⎝x ⎠\
+”“”
+ assert u == s
+
+ u = upretty({x**2: 1})
+ s = \
+u”“”\
+⎧ 2 ⎫
+⎨x : 1⎬
+⎩ ⎭\
+”“”
+ assert u == s
+
record this change to ‘sympy/printing/tests/test_pretty_unicode.py’? [Ynsfdaq?] y
entering patch description...
pretty: fix xobj for even height
Previously pprint({x**2: 1}) was failing with cryptic exception
ValueError: xobj: expect length = 2*k+1
And this is a wrong approach.
Things need to just work, so I’ve modified xobj to tweak evenly heighted
objects somewhat. After all it looks good, so why not?
In [1]: [x**2]
Out[1]:
⎡ 2⎤
⎣x ⎦
In [2]: (x**2,)
Out[2]:
⎛ 2⎞
⎝x ⎠
In [3]: {x**2: 1}
Out[3]:
⎧ 2 ⎫
⎨x : 1⎬
⎩ ⎭
HG: Enter commit message. Lines beginning with ‘HG:’ are removed.
HG: –
HG: user: Kirill Smelkov <kirr@landau.phys.spbu.ru>
HG: branch ‘default’
HG: changed sympy/printing/pretty/pretty_symbology.py
HG: changed sympy/printing/tests/test_pretty_unicode.py
Congrats, now we have two patches recorded:
$ hg qapplied pprint-fix-old-classes.patch xobj-fix-even-height.patch
and workdir is left with only spelling fixes:
$ hqq # 5.patch
Let’s finish it
$ hg qrecord -e printer-fix-typos.patch
diff –git a/sympy/printing/printer.py b/sympy/printing/printer.py
2 hunks, 4 lines changed
examine changes to ‘sympy/printing/printer.py’? [Ynsfdaq?] y
@@ -22,7 +22,7 @@
Also, if BAR is a subclass of FOO, _print_FOO(bar) will be called for
instance of BAR, if no _print_BAR is provided. Thus, usually, we don’t
- need to provide prining routines for every class we want to support –
+ need to provide printing routines for every class we want to support –
only generic routine has to be provided for a set of classes.
A good example for this are functions - for example PrettyPrinter only
record this change to ‘sympy/printing/printer.py’? [Ynsfdaq?] y
@@ -52,7 +52,7 @@
It’s job is to loop through expr classes (class + it’s bases), and
try to dispatch the work to _print_<EXPR_CLASS>
- e.g., suppose we have the following class hierarcy::
+ e.g., suppose we have the following class hierarchy::
Basic
|
record this change to ‘sympy/printing/printer.py’? [Ynsfdaq?] y
printer.py: fix typos HG: Enter commit message. Lines beginning with ‘HG:’ are removed. HG: – HG: user: Kirill Smelkov <kirr@landau.phys.spbu.ru> HG: branch ‘default’ HG: changed sympy/printing/printer.py
Now we have three patches applied
$ hg qapplied pprint-fix-old-classes.patch xobj-fix-even-height.patch printer-fix-typos.patch
and clean workdir
$ hg st $ hg diff $
Here is a bit of history:
$ hg glog -l4 @ changeset: 1902:dff9895d1d23 | tag: qtip | tag: printer-fix-typos.patch | tag: tip | user: Kirill Smelkov <kirr@landau.phys.spbu.ru> | date: Mon Mar 24 22:44:05 2008 +0300 | summary: printer.py: fix typos | o changeset: 1901:787707e1a28c | tag: xobj-fix-even-height.patch | user: Kirill Smelkov <kirr@landau.phys.spbu.ru> | date: Mon Mar 24 22:44:04 2008 +0300 | summary: pretty: fix xobj for even height | o changeset: 1900:b2321483936b | tag: pprint-fix-old-classes.patch | tag: qbase | user: Kirill Smelkov <kirr@landau.phys.spbu.ru> | date: Mon Mar 24 22:44:04 2008 +0300 | summary: pprint: fix handling of old-style classes (#755) | o changeset: 1899:4962f6641827 | tag: qparent | user: Ondrej Certik <ondrej@certik.cz> | date: Sat Mar 22 13:35:55 2008 +0100 | summary: Failing doctests were fixed. This was triggered by a different ordering. |
and a nice graphical history browser is handy
$ hg view
Yep!
Patches are ready, lets send them for review¶
At this stage we are done with preparing our patches, and it is time to send them for review.
First, let’s rebase them to the latest tip.
For this we go to sympy mirroring repository, see whats new, and pull updates:
$ cd ../sympy/ $ hg in comparing with http://hg.sympy.org/sympy searching for changes changeset: 1900:0bc73e367102 user: Ondrej Certik <ondrej@certik.cz> date: Sat Mar 22 16:06:47 2008 +0100 summary: The print ordering of Add has been improved. changeset: 1901:7cd8948a664a user: Kirill Smelkov <kirr@landau.phys.spbu.ru> date: Mon Mar 24 00:17:12 2008 +0300 summary: [1/2] let’s use __slots__ changeset: 1902:529ba5e4a7c6 user: Kirill Smelkov <kirr@landau.phys.spbu.ru> date: Mon Mar 24 00:17:13 2008 +0300 summary: [2/2] let’s use __slots__ changeset: 1903:115df7b1ee75 user: Kirill Smelkov <kirr@landau.phys.spbu.ru> date: Mon Mar 24 00:17:14 2008 +0300 summary: add comments to Mul.flatten changeset: 1904:75544c92be1d tag: tip user: Kirill Smelkov <kirr@landau.phys.spbu.ru> date: Mon Mar 24 01:19:27 2008 +0300 summary: slightly speedup Basic.__getattr__ $ hg pull –update pulling from http://hg.sympy.org/sympy searching for changes adding changesets adding manifests adding file changes added 5 changesets with 26 changes to 21 files 21 files updated, 0 files merged, 0 files removed, 0 files unresolved
Now let’s get back to sympy-fix755
$ cd ../sympy-fix755/
We have three patches applied
$ hg qapplied pprint-fix-old-classes.patch xobj-fix-even-height.patch printer-fix-typos.patch
Let’s rebase them.
First we unapply all patches
$ hg qpop -a Patch queue now empty
Then pull recent changes in from our mirror repo
$ hg pull -u pulling from /home/kirr/src/sympy/spt-work/sympy searching for changes adding changesets adding manifests adding file changes added 5 changesets with 26 changes to 21 files 21 files updated, 0 files merged, 0 files removed, 0 files unresolved
Then apply patches back
$ hg qpush –all applying pprint-fix-old-classes.patch applying xobj-fix-even-height.patch applying printer-fix-typos.patch Now at: printer-fix-typos.patch
Now we have our patches applied on top of the latest tip.
Note
We were lucky.
Sometimes changes we’ve made overlap with the changes made by other developers. This situation is called conflict and has to be resolved using automated merge programs or by hands.
This topic is out of scope of this tutorial, but an interested reader is encouraged to read chapter 3 in hgbook and TutorialConflict HG wiki page.
Ok, it time to send patches for review!
Ensure last time on outgoing changes
$ hg out comparing with /home/kirr/src/sympy/spt-work/sympy searching for changes changeset: 1905:95b5af645143 tag: pprint-fix-old-classes.patch tag: qbase user: Kirill Smelkov <kirr@landau.phys.spbu.ru> date: Wed Mar 26 15:18:09 2008 +0300 summary: pprint: fix handling of old-style classes (#755) changeset: 1906:316070047993 tag: xobj-fix-even-height.patch user: Kirill Smelkov <kirr@landau.phys.spbu.ru> date: Wed Mar 26 15:18:09 2008 +0300 summary: pretty: fix xobj for even height changeset: 1907:63eb63c9f098 tag: qtip tag: printer-fix-typos.patch tag: tip user: Kirill Smelkov <kirr@landau.phys.spbu.ru> date: Wed Mar 26 15:18:09 2008 +0300 summary: printer.py: fix typos
And let’s finally send them:
$ hg email –outgoing comparing with /home/kirr/src/sympy/spt-work/sympy searching for changes This patch series consists of 3 patches. Subject: [PATCH 0 of 3] Fix for #755 + two more Write the introductory message for the patch series. While working on #755 (pprint fails on old style classes) I’ve spot a couple of other inconveniences. Please review. To: sympy-patches@googlegroups.com TODO put this into .hgrc Cc: Sending [PATCH 0 of 3] Fix for #755 + two more ... Sending [PATCH 1 of 3] pprint: fix handling of old-style classes (#755) ... Sending [PATCH 2 of 3] pretty: fix xobj for even height ... Sending [PATCH 3 of 3] printer.py: fix typos ...
Ok, patches sent.
Usually I use Mutt, but I’m having temporary problems with mail delivery today, so this is sympy-patches as viewed from Google web interface:
Ondrej already reviewed them, and all patches were approved. Good.
Two possibilities exist here:
- patches may be applied by reviewer, and
- patches may be pushed in by the author
Generally when you just begin contributing to SymPy, your patches will be applied by someone else, but as your experience matures, you’ll be granted priviledges to push in yourself.
Anyway, let’s see how pushing is done:
Let’s first push to local sympy mirror
$ hg push –force pushing to /home/kirr/src/sympy/spt-work/sympy searching for changes adding changesets adding manifests adding file changes added 3 changesets with 5 changes to 4 files
Note
--force is used here because otherwise hg push complains that source has mq patches applied. As we still push to local repository we know what we do.
Note
If you want to convert MQ patches to regular commits in the same repository, use “hg qdelete -r qbase:qtip” (the patches need to be applied).
Now let’s go to sympy mirror and push into main repo!
$ cd ../sympy $ hg push ssh://hg@hg.sympy.org/sympy pushing to ssh://hg@hg.sympy.org/sympy searching for changes remote: adding changesets remote: adding manifests remote: adding file changes remote: added 3 changesets with 5 changes to 4 files remote: notify: sending 1 subscribers 1 changes remote: notify: sending 1 subscribers 1 changes remote: notify: sending 1 subscribers 1 changes
Ok, patches are pushed. Here is what http://hg.sympy.org/sympy/ says:
Now, let’s close #755
And we are done!
Epilogue¶
Let’s summarize the approach:
- First relax, and hack on the problem as you see fit
- When you think you’ve reached the point when something is done, start recording patches
- After your patches are prepared, send them for review
- If everything is ok, patches go to the main repository.
Interested readers are suggested to read additional literature:
- Mercurial Book, and
- its chapters about mercurial queues: (I, II)
- Mercurial wiki pages: Tutorial, Mq Tutorial, Mercurial Queues,
And of course
Patches Welcome!
We’ve reached the end of this tutorial, now it’s time to practice on the SymPy project: http://www.sympy.org/
We encourage you to join us. Feel free to improve SymPy and this tutorial as well.
Have Fun!
| [1] | http://lkml.org/lkml/2000/8/25/132 |
| [2] | If you use some other editor, just figure out how to make your life as easy as I did for vim (hint Emacs has this for sure too) |
| [3] | I have these aliases in my ~/.bashrc alias hq='hg diff . | vim -R -' alias hqq='hg diff . | gvim -geometry 80x40+600+0 -R "+set nowrap" -' |