On Tue, May 19, 2009 at 5:12 PM, Aaron S. Meurer <asmeu...@gmail.com> wrote: > > > On May 19, 2009, at 5:14 PM, Ondrej Certik wrote: > >> >> Hi Aaron, >> >> It looks good in general, thanks for all the work you already did. >> >> I have some comments below: >> >> 1) One doctest fails, I guess that could be fixed easily: >> >> >> _________________________ sympy.solvers.solvers.dsolve >> _________________________ >> File "/home/ondrej/repos/sympy/sympy/solvers/solvers.py", line 483, in >> sympy.solvers.solvers.dsolve >> Failed example: >> f_ = Derivative(f, x) # f_ will be the derivative of >> Expected: >> f with respect to x >> Got nothing >> ********************************************************************** >> File "/home/ondrej/repos/sympy/sympy/solvers/solvers.py", line 505, in >> sympy.solvers.solvers.dsolve >> Failed example: >> dsolve(Derivative(f(x),x,x)+9*f(x), f(x)) >> Expected: >> f(x) = C1*sin(3*x) + C2*cos(3*x) >> Got: >> f(x) == C1*sin(3*x) + C2*cos(3*x) >> ********************************************************************** >> File "/home/ondrej/repos/sympy/sympy/solvers/solvers.py", line 507, in >> sympy.solvers.solvers.dsolve >> Failed example: >> dsolve(Eq(Derivative(f(x),x,x)+9*f(x)+1, 1), f(x)) >> Expected: >> f(x) = C1*sin(3*x) + C2*cos(3*x) >> Got: >> f(x) == C1*sin(3*x) + C2*cos(3*x) >> >> ============= tests finished: 213 passed, 1 failed in 2.33 seconds >> ============= >> DO *NOT* COMMIT! >> >> >> run "bin/doctest" for just doctests or "./setup.py test" to execute >> tests+doctests. >> > I did not know about doctests. I guess those are different from ./bin/ > test. I think this one is the result of changing dsolve to return an > equality.
Yep, doctests test examples in the docstrings. Those should be used to make it easy for the user to understand what a particular function/method is doing. Regular tests should be used for testing all possibilities. >> 2) If possible, please write longer logs in the patches, so that it's >> clear what your intention is. For example in the patch in 3) below, I >> have hard time thinking what you actually fixed (e.g. there is no test >> and no log). >> > OK. That comment and the docstring should have helped you to figure > it out I hope. I made it try to take the derivative of the solution > if it isn't one that could be explicitly solved, as substituting > doesn't work in that case. I will note these things better in the > logs in the future. It was my fault, the patch is fine. Sorry about it. >> >> 3) thoroughly test everything new that you implement. For example, you >> added new functionality in this patch: >> >> http://github.com/asmeurer/sympy/commit/e57957c648efbfa78f51e6d13233e0045420d5c6 >> >> but I couldn't find the corresponding test for it. > This *is* a test. The function checksol tests solutions to > differential equations to see if they are correct. Every one of the > tests in test_ode uses this function, so if it fails, they fail. Oh, I see now. Sorry, it was my fault. The patch is fine, you can leave it as is. >> >> 4) In this patch: >> >> http://github.com/asmeurer/sympy/commit/82dd5752e877be6c3a939d8f1c9e127b92df9350 >> >> I don't like the way errors in solve() are handled. I think if solve() >> can'd do it, it should raise NotImplementedError() for now (later we >> may even return RootOf, as we are discussing in the list, but for now >> let's just stick to NotImplementedError). I don't think it should ever >> check for AssertionError or TypeError. These mean there is a bug in >> solve(). If you found such a bug, just create a new issue for it (or >> even fix it, if it's trivial). >> > I think that it tried solving some of the equations and it returned > these, so I added them to the list. The ones that returned that > should be among the tests. I would much prefer just calling solve and > letting it return RootOf if it can't do it. > I got AssertionError from test 1 in test_ode15(): > > >>> C1 = Symbol('C1') > >>> solve(Eq(x*cos(y)+y**3/3,C1), y) > Traceback (most recent call last): > File "<console>", line 1, in <module> > File "./sympy/solvers/solvers.py", line 235, in solve > result = tsolve(f, *symbols) > File "./sympy/solvers/solvers.py", line 873, in tsolve > assert False, 'tsolve: at least one Function expected at this > point' > AssertionError: tsolve: at least one Function expected at this point > > I will post an issue. I have no idea why it does this, as I have no > idea how tsolve works. It's clearly a bug. Please create an issue for it. It should be investigated why it does that and fixed. If you want to try it, you can use a debugger, or print statements to figure out why. > > I think TypeError might have been a mistake on my part. I think I > tried doing a solve(eq, f(x)) instead of solve(eq, y). So that > particular patch can be thrown out. Removing it still passes the tests. Ok. > > Once solve returns RootOf all the time, I can remove this whole patch > of code. In the mean while, should I just leave that AssertionError > in there or take it out and make that test an XFAIL? Take the AssertionError out and make the test XFAIL or comment it out. >> 5) in general, it's easier for reviewing if you just rebase the >> patches to the latest master, just do "git rebase master". One should >> not rebase if someone else start using your branch (because rebasing >> makes it really painful for merging new changes), but if it's just >> your own branch, that only you use for developing, it's easier for all >> the other people if you rebase. > > I read that it can be preferable if you do all developing in a branch > and just use master for pulling. This is what I have been doing. git > checkout master; git pull; git checkout odes; git merge master. Will Just use "git rebase master" instead of "git merge master". And that's it. > rebasing make things harder when I have to merge from the main repo? It's the same easy. >> But apart from that, I like it. You can fix the above just by doing >> "git rebase -i" and edit those patches. Ondrej --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "sympy-patches" group. To post to this group, send email to sympy-patches@googlegroups.com To unsubscribe from this group, send email to sympy-patches+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/sympy-patches?hl=en -~----------~----~----~----~------~----~------~--~---
