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.

Sign up for GitHub

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

Jump to bottom

modified 'get_numbered_constants' and test 9239, added another test #15504

Merged
merged 4 commits into from Nov 26, 2018
Merged

modified 'get_numbered_constants' and test 9239, added another test #15504

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d04f808
modified 'get_numbered_constants and test 9239, added another test
avishrivastava11 Nov 18, 2018
71c5019
removed extra blank lines from test_ode.py
avishrivastava11 Nov 21, 2018
feac392
removed unnecessary blanks from import statement
avishrivastava11 Nov 21, 2018
47b9f94
shortening import lines
avishrivastava11 Nov 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions sympy/solvers/ode.py
View file
Open in desktop
Expand Up @@ -395,6 +395,9 @@ def get_numbered_constants(eq, num=1, start=1, prefix='C'):
raise ValueError("Expected Expr or iterable but got %s" % eq)

atom_set = set().union(*[i.free_symbols for i in eq])
func_set = set().union(*[i.atoms(Function) for i in eq])
if func_set:
atom_set |= {Symbol(str(f.func)) for f in func_set}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's an example of where #15460 would apply: if all functions had a name attribute then this could be Symbol(f.name). But if no function has a name attribute (as is the case in this PR), then what you have done here makes sense and will work even if a non-Undefined functions happened to have a number in the name (like log2).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smichr Yes that makes sense. But looking at the discussion in #15460 I don't think you or @asmeurer (or in this case, me too) agree to have all the functions return a name attribute. So introducing that change (Symbol(f.name)) wouldn't be necessary unless we give all the functions a name attribute.

Copy link
Contributor

@cbm755 cbm755 Nov 21, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@avishrivastava11 can you state whether

  1. you did this work independently
    or
  2. you read someone else's code to come up with this fix.

Its not a big deal either way (and this is not an accusation---I'm just trying to keep and clean open record of our development).

If you did read someone else's code to come up with this fix, then perhaps I would prefer to cherry-pick a commit from elsewhere (and then rebase your tests on top of it).

@asmeurer please do not merge until we resolve this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cbm755 I assure you, I'm completely against plagiarism. I did this independently (and actually have been on the receiving end of someone else using my ideas, so I know how it feels and how bad it is) and therefore appreciate your concern. And yes, whenever I use someone else's commits or work, I first pull their commits in the PR and then work on top of them and also give them credit for it (you can take a look at a few of my previous PRs)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not mean to suggest anything inappropriate had happened; please accept my apologies if you felt threatened by my comments.

@asmeurer an equivalent change was done in Diofant (which Github has linked to my original bug report). We could've simply cherry-picked that change but no one did so. Instead, we have what I believe to be an independently written solution (based on a different variable name and a clear statement from the author).

I'm now +1 for this change after minor reformating.

ncs = numbered_symbols(start=start, prefix=prefix, exclude=atom_set)
Cs = [next(ncs) for i in range(num)]
return (Cs[0] if num == 1 else tuple(Cs))
Expand Down
19 changes: 10 additions & 9 deletions sympy/solvers/tests/test_ode.py
View file
Open in desktop
Expand Up @@ -5,10 +5,10 @@
Dummy, Eq, Ne, erf, erfi, exp, Function, I, Integral, LambertW, log, O, pi,
Rational, rootof, S, simplify, sin, sqrt, Subs, Symbol, tan, asin, sinh,
Piecewise, symbols, Poly, sec, Ei)
from sympy.solvers.ode import (_undetermined_coefficients_match, checkodesol,
classify_ode, classify_sysode, constant_renumber, constantsimp,
homogeneous_order, infinitesimals, checkinfsol, checksysodesol, solve_ics,
dsolve)
from sympy.solvers.ode import (_undetermined_coefficients_match,
checkodesol, classify_ode, classify_sysode, constant_renumber,
constantsimp, homogeneous_order, infinitesimals, checkinfsol,
checksysodesol, solve_ics, dsolve, get_numbered_constants)
from sympy.solvers.deutils import ode_order
from sympy.utilities.pytest import XFAIL, skip, raises, slow, ON_TRAVIS

Expand Down Expand Up @@ -2957,20 +2957,21 @@ def test_dsolve_linsystem_symbol():
Eq(g(x), -C1*eps*sin(eps*x) + C2*eps*cos(eps*x))]
assert checksysodesol(eq1, sol1) == (True, [0, 0])



def test_C1_function_9239():
t = Symbol('t')
C1 = Function('C1')
C2 = Function('C2')
C3 = Symbol('C1') # XXX: update these after
C4 = Symbol('C2') # XXX: https://github.com/sympy/sympy/issues/15056
C3 = Symbol('C3')
C4 = Symbol('C4')
eq = (Eq(diff(C1(t), t), 9*C2(t)), Eq(diff(C2(t), t), 12*C1(t)))
sol = [Eq(C1(t), 9*C3*exp(6*sqrt(3)*t) + 9*C4*exp(-6*sqrt(3)*t)),
Eq(C2(t), 6*sqrt(3)*C3*exp(6*sqrt(3)*t) - 6*sqrt(3)*C4*exp(-6*sqrt(3)*t))]
assert checksysodesol(eq, sol) == (True, [0, 0])


def test_issue_15056():
t = Symbol('t')
C3 = Symbol('C3')
assert get_numbered_constants(Symbol('C1') * Function('C2')(t)) == C3

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why this file has so much empty space, but there should be only one or two blank lines between functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed a few blank lines from the tests/test_ode.py file. I've not gone about removing all the extra blank lines as that's not the purpose of this PR.

def test_issue_10379():
t,y = symbols('t,y')
Expand Down