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

avishrivastava11 · 2018-11-18T11:01:17Z

modified 'get_numbered_constants' and test_C1_function_9239
added another test

Earlier

from sympy.solves.ode import get_numbered_constants
get_numbered_constants(Symbol('C1')*Function('C2')(t))

gave Symbol('C2') which is technically ok but could lead to confusion (see test_C1_function_9239 in the master branch; it's been updated in this PR).
It's been corrected now and it returns Symbol('C3')

Release Notes

solvers
- get_numbered_constants in dsolve now considers functions, in case you have a function named, for example, C1(t).

sympy-bot · 2018-11-18T11:01:25Z

✅

Hi, I am the SymPy bot (v134). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

solvers
- get_numbered_constants in dsolve now considers functions, in case you have a function named, for example, C1(t). (#15504 by @avishrivastava11)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.4.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

Click here to see the pull request description that was parsed.

modified 'get_numbered_constants' and test_C1_function_9239
added another test

Fixes #15056 

Earlier 
```
from sympy.solves.ode import get_numbered_constants
get_numbered_constants(Symbol('C1')*Function('C2')(t))
``` 
gave `Symbol('C2')` which is technically ok but could lead to confusion (see `test_C1_function_9239` in the `master branch`; it's been updated in this PR).
It's been corrected now and it returns `Symbol('C3')`


#### Release Notes

<!-- BEGIN RELEASE NOTES -->
* solvers
    * `get_numbered_constants` in `dsolve` now considers functions, in case you have a function named, for example, `C1(t)`.
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

smichr · 2018-11-18T15:36:11Z

sympy/solvers/ode.py

@@ -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}


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).

@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.

@avishrivastava11 can you state whether

you did this work independently
or

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.

@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)

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.

avishrivastava11 · 2018-11-19T13:15:34Z

The failing test in d04f808 commit is passing on my local system :

avi@avi-Aspire-A515-51G:~/sympy$ git branch
* get_numbered_constants
  master
avi@avi-Aspire-A515-51G:~/sympy$ python3
Python 3.6.6 (default, Sep 12 2018, 18:26:19) 
[GCC 8.0.1 20180414 (experimental) [trunk revision 259383]] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import sympy
>>> sympy.test("sympy/solvers/tests/test_solvers.py")
============================= test process starts ==============================
executable:         /usr/bin/python3  (3.6.6-final-0) [CPython]
architecture:       64-bit
cache:              yes
ground types:       python 
numpy:              1.15.1
random seed:        49800762
hash randomization: on (PYTHONHASHSEED=916085924)

sympy/solvers/tests/test_solvers.py[104] .............w........................w
wf...........................f.f..................w..............           [OK]

________________________________ slowest tests _________________________________
test_lambert_multivariate - Took 12.446 seconds
test_solve_transcendental - Took 18.578 seconds
test_issue_2840_8155 - Took 18.586 seconds
test_issue_12114 - Took 18.785 seconds
test_issue_5114 - Took 28.576 seconds
test_high_order_multivariate - Took 247.398 seconds
= tests finished: 97 passed, 4 skipped, 3 expected to fail, in 424.08 seconds ==
True
>>>

Can someone restart the failing test?

avishrivastava11 · 2018-11-19T13:56:31Z

@smichr @cbm755 Does this look good to merge now?

asmeurer · 2018-11-20T16:56:31Z

Release notes should describe what is changed for the point of view of the end user (for example, "get_numbered_constants now considered functions").

asmeurer · 2018-11-20T16:56:41Z

The changes themselves look good, though.

asmeurer · 2018-11-20T16:57:35Z

sympy/solvers/tests/test_ode.py

+    t = Symbol('t')
+    C3 = Symbol('C3')
+    assert get_numbered_constants(Symbol('C1') * Function('C2')(t)) == C3
+


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

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.

avishrivastava11 · 2018-11-21T13:28:14Z

@asmeurer @smichr @cbm755 If there aren't anymore changes you'd like to suggest, please merge.

avishrivastava11 · 2018-11-21T17:36:27Z

Can someone please restart the failing builds? (the tests should pass since there's been no change other than removal of a few blank lines)

cbm755 · 2018-11-21T18:18:44Z

sympy/solvers/tests/test_ode.py

-    dsolve)
+                               classify_ode, classify_sysode, constant_renumber, constantsimp,
+                               homogeneous_order, infinitesimals, checkinfsol, checksysodesol, solve_ics,
+                               dsolve, get_numbered_constants)


now these lines look too long to me, past column 80? Maybe just leave the local formatting alone?

Yes, I should've seen that. PyCharm actually does some addition of lines and spaces while automatically adding a needed import. Removing them now.

Done. Please take a look

cbm755 · 2018-11-21T18:28:42Z

I restarted the job and I edited your release notes; please double-check

avishrivastava11 · 2018-11-22T03:07:17Z

Again, one of the tests fail, although works perfectly on my local system.

avi@avi-Aspire-A515-51G:~/sympy$ python3
Python 3.6.6 (default, Sep 12 2018, 18:26:19) 
[GCC 8.0.1 20180414 (experimental) [trunk revision 259383]] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import sympy
>>> sympy.test("sympy/core/tests/test_sympify.py")
============================= test process starts ==============================
executable:         /usr/bin/python3  (3.6.6-final-0) [CPython]
architecture:       64-bit
cache:              yes
ground types:       python 
numpy:              1.15.1
random seed:        37194411
hash randomization: on (PYTHONHASHSEED=3223196904)

sympy/core/tests/test_sympify.py[44] .........................................f.
.                                                                           [OK]

======== tests finished: 43 passed, 1 expected to fail, in 0.52 seconds ========
True

Please, someone restart the failing job.

asmeurer · 2018-11-22T03:40:20Z

The job will fail until #15523 is merged.

avishrivastava11 · 2018-11-22T04:23:21Z

@asmeurer Since #15523 has been merged now, please restart the failing job.

cbm755 · 2018-11-22T07:20:10Z

sympy/solvers/tests/test_ode.py

-    homogeneous_order, infinitesimals, checkinfsol, checksysodesol, solve_ics,
-    dsolve)
+    classify_ode, classify_sysode, constant_renumber, constantsimp, homogeneous_order,
+    infinitesimals, checkinfsol, checksysodesol, solve_ics, dsolve, get_numbered_constants)


sorry but these lines are still too long

avishrivastava11 · 2018-11-22T10:40:12Z

@asmeurer Should the changes be merged now?

modified 'get_numbered_constants and test 9239, added another test

d04f808

smichr reviewed Nov 18, 2018

View reviewed changes

smichr mentioned this pull request Nov 18, 2018

name now works correctly for undefined functions #15460

Merged

asmeurer reviewed Nov 20, 2018

View reviewed changes

removed extra blank lines from test_ode.py

71c5019

cbm755 reviewed Nov 21, 2018

View reviewed changes

removed unnecessary blanks from import statement

feac392

cbm755 reviewed Nov 22, 2018

View reviewed changes

shortening import lines

47b9f94

asmeurer merged commit 0d5e084 into sympy:master Nov 26, 2018

avishrivastava11 deleted the get_numbered_constants branch December 14, 2018 06:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

avishrivastava11 commented Nov 18, 2018 •

edited by cbm755

sympy-bot commented Nov 18, 2018 •

edited

smichr Nov 18, 2018

avishrivastava11 Nov 19, 2018

cbm755 Nov 21, 2018 •

edited

avishrivastava11 Nov 21, 2018 •

edited

cbm755 Nov 22, 2018

avishrivastava11 commented Nov 19, 2018

avishrivastava11 commented Nov 19, 2018

asmeurer commented Nov 20, 2018

asmeurer commented Nov 20, 2018

asmeurer Nov 20, 2018

avishrivastava11 Nov 21, 2018

avishrivastava11 commented Nov 21, 2018

avishrivastava11 commented Nov 21, 2018

cbm755 Nov 21, 2018

avishrivastava11 Nov 21, 2018

avishrivastava11 Nov 21, 2018

cbm755 commented Nov 21, 2018

avishrivastava11 commented Nov 22, 2018

asmeurer commented Nov 22, 2018

avishrivastava11 commented Nov 22, 2018

cbm755 Nov 22, 2018

avishrivastava11 Nov 22, 2018

avishrivastava11 commented Nov 22, 2018

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

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

Conversation

avishrivastava11 commented Nov 18, 2018 • edited by cbm755

Release Notes

sympy-bot commented Nov 18, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbm755 Nov 21, 2018 • edited

Choose a reason for hiding this comment

avishrivastava11 Nov 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avishrivastava11 commented Nov 19, 2018

avishrivastava11 commented Nov 19, 2018

asmeurer commented Nov 20, 2018

asmeurer commented Nov 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avishrivastava11 commented Nov 21, 2018

avishrivastava11 commented Nov 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbm755 commented Nov 21, 2018

avishrivastava11 commented Nov 22, 2018

asmeurer commented Nov 22, 2018

avishrivastava11 commented Nov 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avishrivastava11 commented Nov 22, 2018

avishrivastava11 commented Nov 18, 2018 •

edited by cbm755

sympy-bot commented Nov 18, 2018 •

edited

cbm755 Nov 21, 2018 •

edited

avishrivastava11 Nov 21, 2018 •

edited