Implement Symbolic Factorial and Binomial #1337

ShikharJ · 2017-10-04T09:22:41Z

Relevant: #1336

isuruf · 2017-10-04T16:03:44Z

symengine/functions.cpp

+    } else if (is_a<Integer>(*arg)
+               or (is_a_Number(*arg)
+                   and not down_cast<const Number &>(*arg).is_exact())) {
+        return gamma(add(arg, one));


There's a factorial in ntheory that you can use here.

My concern is regarding code duplication that would occur. gamma already has the cases defined. It would be a literal copy-paste.

This returns an instance of Gamma sometimes. Can you return a Factorial instance in that case?

ShikharJ · 2018-01-14T11:17:01Z

@isuruf I think this has progressed enough for a review.

ShikharJ · 2018-01-29T04:00:43Z

@isuruf Can I please have a review of this?

isuruf · 2018-01-29T04:05:03Z

symengine/functions.cpp

+                        *integer(2)))) {
+            return gamma(add(arg, one));
+        }
+    }


SymEngine's gamma and sympy's factorial evaluates this for floating points. How about just returning gamma(arg + 1)?

Floats are being evaluated here, these checks are just to prevent from returning instances of Gamma. See here.

isuruf · 2018-01-29T04:09:25Z

symengine/functions.cpp

+                            down_cast<const Integer &>(*k).as_uint());
+        }
+        if (is_a<Rational>(*n) or is_a<RealDouble>(*n)) {
+            if (down_cast<const Number &>(*sub(n, k)).is_negative()) {


Move this to under is_a_Number(*n) and is_a_Number(*k)

isuruf · 2018-01-29T04:10:05Z

symengine/functions.cpp

+    if (is_a_Number(*k) and down_cast<const Number &>(*k).is_zero()) {
+        return one;
+    } else if (is_a_Boolean(*n) or is_a_Boolean(*k) or is_a_Set(*n)
+               or is_a_Set(*k)) {


Don't check this. Garbage values in -> garbage values out

isuruf · 2018-01-29T04:11:08Z

symengine/functions.cpp

+        return one;
+    } else if (is_a<NaN>(*n)) {
+        if ((is_a<Integer>(*k)
+             and down_cast<const Number &>(*k).is_positive())) {


Why? Just return NaN if any one of n or k is a NaN

This is SymPy's output:

In [5]: binomial(nan, x) Out[5]: ⎛nan⎞ ⎜ ⎟ ⎝ x ⎠ In [6]: binomial(nan, 3/2) Out[6]: ⎛nan⎞ ⎜ ⎟ ⎝1.5⎠ In [7]: binomial(nan, -3) Out[7]: ⎛nan⎞ ⎜ ⎟ ⎝ -3⎠

If one of the arguments is NaN, the value of the function doesn't make sense. Therefore a NaN should be returned,

isuruf · 2018-01-29T04:12:59Z

symengine/functions.cpp

+    } else if (is_same_type(*n, *k)) {
+        if (is_a<Integer>(*n)) {
+            return binomial(down_cast<const Integer &>(*n),
+                            down_cast<const Integer &>(*k).as_uint());


As this is the most important case, move it to the top of if-else

isuruf · 2018-01-29T04:14:18Z

symengine/functions.cpp

+               or is_a_Set(*k)) {
+        throw SymEngineException(
+            "Boolean and Set objects not allowed in this context.");
+    } else if (eq(*n, *k) and not(is_a<NaN>(*n) or is_a<Infty>(*n))) {


Move the NaN check to above this, so that you don't have to check here again.

isuruf · 2018-01-29T04:17:13Z

symengine/printer.cpp

+    str_ = s.str();
+}
+
+void StrPrinter::bvisit(const Binomial &x)


Don't need a special method here. See how sin and others are handled

isuruf · 2018-01-29T04:17:41Z

symengine/printer.cpp

+void StrPrinter::bvisit(const Factorial &x)
+{
+    std::ostringstream s;
+    s << "(" << apply(x.get_arg()) << ")!";


x! should not be printed as (x)!

This was actually done for these cases.

I mean you should check the precedence of the arg before printing the parenthesis. Take a look at how parenthesis are added. For example, x**n vs (x+y)**n where there is no parenthesis around x in the first case

isuruf · 2018-01-29T16:55:20Z

symengine/functions.cpp

+            return zero;
+        }
+    } else if (is_a_Number(*k) and down_cast<const Number &>(*k).is_zero()) {
+        return one;


Also return 0 if k is a number and k < 0.

isuruf · 2018-01-29T16:59:08Z

symengine/functions.cpp

+}
+
+RCP<const Basic> binomial(const RCP<const Basic> &n, const RCP<const Basic> &k)
+{


Here first check that k is an integer. If k is not an integer, return a Binomial object.

sudoWin · 2021-01-14T11:39:29Z

@isuruf Can I take up this issue in a new PR?

I guess only the conflicts need to be corrected.

ShikharJ · 2021-01-15T16:03:11Z

@sudoWin Go for it.

isuruf reviewed Oct 4, 2017

View reviewed changes

ShikharJ added the Hacktoberfest label Oct 18, 2017

ShikharJ force-pushed the Combinatorics branch from 5add636 to 1f41231 Compare October 22, 2017 08:57

Implement Symbolic Factorial

c3a3e53

ShikharJ force-pushed the Combinatorics branch from 1f41231 to 8fafda5 Compare December 20, 2017 20:06

ShikharJ force-pushed the Combinatorics branch 2 times, most recently from a6245b6 to 8fa036d Compare January 14, 2018 11:14

ShikharJ changed the title ~~[WIP] Implement Symbolic Factorial and Binomial~~ Implement Symbolic Factorial and Binomial Jan 14, 2018

ShikharJ requested a review from isuruf January 14, 2018 11:17

ShikharJ force-pushed the Combinatorics branch from 8fa036d to 7ba7150 Compare January 14, 2018 22:10

Implement Symbolic Binomial

7ba7150

isuruf reviewed Jan 29, 2018

View reviewed changes

Improvements

91f1503

ShikharJ force-pushed the Combinatorics branch from 29b5d87 to 91f1503 Compare January 29, 2018 20:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Symbolic Factorial and Binomial #1337

Implement Symbolic Factorial and Binomial #1337

ShikharJ commented Oct 4, 2017

isuruf Oct 4, 2017

ShikharJ Oct 4, 2017

isuruf Oct 4, 2017

isuruf Oct 4, 2017

ShikharJ commented Jan 14, 2018

ShikharJ commented Jan 29, 2018

isuruf Jan 29, 2018

ShikharJ Jan 29, 2018

isuruf Jan 29, 2018

isuruf Jan 29, 2018

isuruf Jan 29, 2018

ShikharJ Jan 29, 2018

isuruf Jan 29, 2018

isuruf Jan 29, 2018

isuruf Jan 29, 2018

isuruf Jan 29, 2018

isuruf Jan 29, 2018

ShikharJ Jan 29, 2018

isuruf Jan 29, 2018

isuruf Jan 29, 2018

isuruf Jan 29, 2018

sudoWin commented Jan 14, 2021

ShikharJ commented Jan 15, 2021

Implement Symbolic Factorial and Binomial #1337

Are you sure you want to change the base?

Implement Symbolic Factorial and Binomial #1337

Conversation

ShikharJ commented Oct 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShikharJ commented Jan 14, 2018

ShikharJ commented Jan 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sudoWin commented Jan 14, 2021

ShikharJ commented Jan 15, 2021