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

added is_number property for meijerg #13558

Merged
merged 3 commits into from Nov 4, 2017
Merged

Conversation

ashishkg0022
Copy link
Contributor

meijerg.is_number was giving a wrong answer according to issue #13071. so I added a new property for it.

@ashishkg0022
Copy link
Contributor Author

@jksuom can you please review it ?

@jksuom
Copy link
Member

jksuom commented Nov 3, 2017

There is an example in core.expr.is_number that contains a symbol:

>>> (2 + Integral(2, (x, 1, 2))).is_number
True
>>> (2 + Integral(2, (x, 1, 2))).atoms(Symbol)
set([x])

Perhaps the test should be modified to ignore bound symbols such as x in the integral above.

@ashishkg0022
Copy link
Contributor Author

Is it fine now ?

@ashishkg0022
Copy link
Contributor Author

@jksuom Is it fine now ?

@jksuom
Copy link
Member

jksuom commented Nov 4, 2017

I think the code is fine, but there could also be a test where the result is not a number.

@ashishkg0022
Copy link
Contributor Author

@jksuom i already have added it . line 126 assert g.is_number is False

@jksuom
Copy link
Member

jksuom commented Nov 4, 2017

That is true. I failed to see that g was also a G-function.

Thanks! I think this is ready.

@jksuom jksuom merged commit 57f80ff into sympy:master Nov 4, 2017
@ashishkg0022 ashishkg0022 deleted the meijerg branch November 5, 2017 03:24
@ashishkg0022
Copy link
Contributor Author

@jksuom I think , I also should modify docstrings (examples) in hyper.py . what do you say ?

@ashishkg0022
Copy link
Contributor Author

@jksuom

 Examples
    ========

    You can pass the parameters either as four separate vectors:

    >>> from sympy.functions import meijerg
    >>> from sympy.abc import x, a
    >>> from sympy.core.containers import Tuple
    >>> from sympy import pprint
    >>> pprint(meijerg((1, 2), (a, 4), (5,), [], x), use_unicode=False)
     __1, 2 /1, 2  a, 4 |  \
    /__     |           | x|
    \_|4, 1 \ 5         |  /

    or as two nested vectors:

    >>> pprint(meijerg([(1, 2), (3, 4)], ([5], Tuple()), x), use_unicode=False)
     __1, 2 /1, 2  3, 4 |  \
    /__     |           | x|
    \_|4, 1 \ 5         |  /

    As with the hypergeometric function, the parameters may be passed as
    arbitrary iterables. Vectors of length zero and one also have to be
    passed as iterables. The parameters need not be constants, but if they
    depend on the argument then not much implemented functionality should be
    expected.

    All the subvectors of parameters are available:

    >>> from sympy import pprint
    >>> g = meijerg([1], [2], [3], [4], x)
    >>> pprint(g, use_unicode=False)
     __1, 1 /1  2 |  \
    /__     |     | x|
    \_|2, 2 \3  4 |  /
    >>> g.an
    (1,)
    >>> g.ap
    (1, 2)
    >>> g.aother
    (2,)
    >>> g.bm
    (3,)
    >>> g.bq
    (3, 4)
    >>> g.bother
    (4,)

In these examples .is_number should be added. Should I add it ?

@ashishkg0022
Copy link
Contributor Author

I just want to add the example.

@jksuom
Copy link
Member

jksuom commented Nov 5, 2017

Is there something wrong or do you just want to add some examples?

@jksuom
Copy link
Member

jksuom commented Nov 5, 2017

It seems that docstring examples are generally focused on explaining the main subject. There is a big number of methods of the type .is_xxxxx and those are added to docstrings only if that would help to clarify the matter. In this case, I don't think that .is_number would be necessary in the docstring of meijerg.

@ashishkg0022
Copy link
Contributor Author

ashishkg0022 commented Nov 5, 2017

Ok. Thanks :)

@gxyd
Copy link
Contributor

gxyd commented Nov 5, 2017

You can use 'Fixes #13071 ' in your commit message. That will close the issue.

@gxyd gxyd mentioned this pull request Nov 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants