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.

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

Fix issue #879: series of undefined functions. #1888

Closed
wants to merge 3 commits into from

Conversation

renatocoutinho
Copy link
Contributor

No description provided.

@asmeurer
Copy link
Member

You might also add a test for a subclass of Function, like

class TestFunction(Function):
    pass

TestFunction(x).series(x)

@@ -483,7 +478,7 @@ def _eval_nseries(self, x, n, logx):
s = s.removeO()
s = s.subs(v, zi).expand() + C.Order(o.expr.subs(v, zi), x)
return s
if (self.func.nargs == 1 and args0[0]) or self.func.nargs > 1:
if ((self.func.nargs == 1 or isinstance(self.func, UndefinedFunction)) and args0[0]) or self.func.nargs > 1:
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this just go in the UndefinedFunction class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

UndefinedFunction is actually a metaclass, so I would have to mess with it a bit. OTOH, would it be really better to repeat most of the series logic in a different place? The algorithm as it is looks fine, I have to special-case it just because f(x).nargs == 1 but f.nargs is None.

Copy link
Member

Choose a reason for hiding this comment

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

I guess AppliedUndef then?

Copy link
Member

Choose a reason for hiding this comment

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

Actually, from what you say, it sounds like what you really should be testing for is the nargs is None case.

Copy link
Member

Choose a reason for hiding this comment

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

So this seems to be wrong anyway. Try f(x, y).series(x).

Copy link
Member

Choose a reason for hiding this comment

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

Yet f(x, y).series(y) gives the right thing.

@asmeurer
Copy link
Member

By the way, great to have you contributing again!

Now we can use also

    >>> class TestF(Function):
    ...     pass
    >>> TestF(x).series(x)
@jrioux
Copy link
Member

jrioux commented Mar 13, 2013

SymPy Bot Summary: 🔴 Failed after merging renatocoutinho/879b (3e9f0ad) into master (6faba52).
@renatocoutinho: Please fix the test failures.
✳️ PyPy 2.0.0-beta-1; 2.7.3-final-42: pass
✳️ Python 2.7.2-final-0: pass
🔴 Python 3.2.1-final-0: fail
✳️ Sphinx 1.1.3: pass
Docs build command: make clean && make html-errors && make latex && cd _build/latex && xelatex sympy-*.tex

@jrioux
Copy link
Member

jrioux commented Mar 14, 2013

SymPy Bot Summary: ✳️ Passed after merging renatocoutinho/879b (ca49718) into master (f556486).
✳️ PyPy 2.0.0-beta-1; 2.7.3-final-42: pass
✳️ Python 2.7.2-final-0: pass
✳️ Python 3.2.1-final-0: pass
✳️ Sphinx 1.1.3: pass
Docs build command: make clean && make html-errors && make latex && cd _build/latex && xelatex sympy-*.tex

@asmeurer
Copy link
Member

SymPy Bot Summary: ✳️ Passed after merging renatocoutinho/879b (ca49718) into master (b498c0f).
✳️ Python 2.5.6-final-0: pass
✳️ Python 2.6.8-final-0: pass
✳️ Python 2.7.3-final-0: pass
✳️ Python 3.2.3-final-0: pass
✳️ Sphinx 1.1.3: pass

@smichr
Copy link
Member

smichr commented Mar 18, 2013

>>> f, g = map(Function, 'fg')
>>> f(x).series(x, a, 3).subs(f, sin).doit()
sin(a) + x*cos(a) - x**2*sin(a)/2 + O(x**3)

And I was expecting sin(a) + (x-a)*cos(a) - (x-a)**2/2*sin(a) + O(x**3). I know that this is because of the reasons noted in the series docstring. But I wonder if we could just use Subs on the O term, too:

if series_result.has(O):
  if series_result.is_Add:
    e, o = series_result.as_independent(O)
    if not e.has(O):
      return e.subs(x, x - x0) + Subs(o, x, x - x0)
return series_result

for example

>>> def go():
...     if series_result.has(O):
...       if series_result.is_Add:
...         e, o = series_result.as_independent(O)
...         if not e.has(O):
...           return e.subs(x, x - x0) + Subs(o, x, x - x0)
...     return series_result
...
>>> x0=a; series_result = sin(x).series(x,x0,n=3)
>>> go()
-(-a + x)**2*sin(a)/2 + (-a + x)*cos(a) + sin(a) + Subs(O(x**3), (x,), (-a + x,))
>>> x0=a;series_result = (x**2).series(x,x0,n=3)
>>> go()
a**2 + 2*a*(-a + x) + (-a + x)**2
>>> series_result
a**2 + 2*a*(-a + x) + (-a + x)**2

@smichr
Copy link
Member

smichr commented Apr 10, 2013

  • rebase
  • respond to suggestion about putting O in Subs

@asmeurer
Copy link
Member

asmeurer commented Jun 2, 2013

  • Make sure functions of multiple variables work and are tested.

@danieljfarrell
Copy link

Can I check the status of this pull request, when do you think it will be added to master?

@asmeurer
Copy link
Member

asmeurer commented Jun 8, 2013

SymPy Bot Summary: ❗ There were merge conflicts (could not merge renatocoutinho/879b (ca49718) into master (ec1ac09)); could not test the branch.
@renatocoutinho: Please rebase or merge your branch with master. See the report for a list of the merge conflicts.

@asmeurer
Copy link
Member

asmeurer commented Jun 8, 2013

Unless I misunderstand this issue, I think the O Subs thing can be postponed. I tested this branch, and it seems to work for functions of multiple variables. It just needs to be tested.

So if someone wants to pick up work here, what needs to be done is to merge the branch with master, and add tests for functions of multiple variables. I think @renatocoutinho is busy, so if you want to pick up the work, just make a new branch based off this one and make a new pull request.

@asmeurer
Copy link
Member

SymPy Bot Summary: ❗ There were merge conflicts (could not merge renatocoutinho/879b (ca49718) into master (71a8f17)); could not test the branch.
@renatocoutinho: Please rebase or merge your branch with master. See the report for a list of the merge conflicts.

@tswast tswast mentioned this pull request Jun 29, 2013
@asmeurer
Copy link
Member

See #2224.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants