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. > 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. > > 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. > > 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. 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. 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? > 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 rebasing make things harder when I have to merge from the main repo? > But apart from that, I like it. You can fix the above just by doing > "git rebase -i" and edit those patches. > > Thanks, > Ondrej Aaron Meurer --~--~---------~--~----~------------~-------~--~----~ 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 -~----------~----~----~----~------~----~------~--~---
