Implement Beta function by abhinavagarwal07 · Pull Request #707 · symengine/symengine

abhinavagarwal07 · 2015-12-26T14:12:11Z

#706

certik · 2015-12-26T16:05:20Z

Space after comma.

certik · 2015-12-26T16:05:33Z

Don't forget to add tests.

isuruf · 2015-12-26T17:26:04Z

Since beta is symmetric, is it necessary to enforce the order of the arguments here?

@isuruf
Thanks for pointing out.
This has been solved.

abhinavagarwal07 · 2015-12-26T17:36:45Z

If I call gamma, it will redo all if statement checks like is a integer or is multiple of 2.
Though I have no other objections against it.
Also note rewrite_as_gamma is also implemented.

sumith1896 · 2015-12-27T10:46:07Z

Do you intent on both being return false ?

abhinavagarwal07 · 2015-12-27T17:16:46Z

@sumith
Sorry. I'll fix it

sumith1896 · 2015-12-27T18:41:04Z

Cool 😄

certik · 2015-12-31T14:40:40Z

Space after Beta.

abhinavagarwal07 · 2016-01-01T15:41:39Z

@certik
ping

abhinavagarwal07 · 2016-01-02T11:23:39Z

@sumith1896
Any suggestions?

sumith1896 · 2016-01-02T12:07:36Z

@abhinavagarwal07 Having a look

sumith1896 · 2016-01-02T12:15:48Z

for ( ... ) {

sumith1896 · 2016-01-02T12:44:18Z

I have no much comments @abhinavagarwal07 . I think you should address Ondrej's comment above.

isuruf · 2016-01-02T13:42:26Z

This method should not throw an error

If none of above 4 if's are satisfied, then how should I compare ?

certik · 2016-01-03T05:34:51Z

@abhinavagarwal07 thanks for the PR, this will be very useful. Please ping me once you address the issues that @isuruf raised.

isuruf · 2016-01-03T17:21:00Z

@certik, @abhinavagarwal07, what do you think about ordering the terms before creating the objects. For eg: beta(y, x) would return beta(x, y)? This way it'll be easier to write correct compare method. The other option would be to sort first before comparing.

abhinavagarwal07 · 2016-01-03T17:24:18Z

@isuruf
yes this can be done but how will you compare
β(3, 4) and β(2, 5)

isuruf · 2016-01-03T17:27:49Z

Assuming the arguments are sorted, I would compare the first element of each object and return the value if they are unequal. If they are equal I will move on to the second argument

abhinavagarwal07 · 2016-01-03T17:31:24Z

Ok. I will do it.

abhinavagarwal07 · 2016-01-04T17:39:54Z

@isuruf thanks for suggestions.
@isuruf @certik ping

isuruf · 2016-01-04T17:56:26Z

This is correct, but if we call beta(y, x) then this will return false, right?
Each is_canonical check has to be enforced by the method that constructs it, in this case beta method. In it there are calls like make_rcp<const Beta>(x, y). These methods have to be changed.

For example, there is a static method called Rational::from_mpq which checks if mpq_class is an integer or not and returns Integer or Rational instance.

For Beta you need to have a method like

static RCP<const Beta> from_two_basic(const RCP<const Basic> &x, const RCP<const Basic> &y)

which would call make_rcp<const Beta>(x, y) or make_rcp<const Beta>(y, x) depending on the order.

add Beta::from_two_basic

certik · 2016-01-05T15:46:10Z

I would like to merge #738 first if it is ok with you. I can then help you fix any merge conflicts in this PR.

abhinavagarwal07 · 2016-01-05T16:21:25Z

Sure.

certik · 2016-01-05T16:45:58Z

Can you merge with the latest master (now when #738) was merged? I think you need to remove the accept method.

abhinavagarwal07 · 2016-01-06T05:49:05Z

@certik
I have merged with master.

seems like trailing whitespace check utility has failed as tests before this commits have passed

certik · 2016-01-06T17:17:07Z

I think that this looks good. Do the tests cover all the if statements in the beta function?

Do you want to tidy the history a bit? I think you can just create a few nice commits that add the Beta function, instead of all the merges and white space fixes. I can help you with that. Submit it as a new PR, so that we have this PR for reference.

isuruf · 2016-01-06T17:18:36Z

You don't need these lines anymore as the terms are ordered

abhinavagarwal07 · 2016-01-07T12:37:22Z

https://github.com/abhinavagarwal07/symengine/tree/beta-function2

abhinavagarwal07 · 2016-01-07T18:13:40Z

#747

abhinavagarwal07 added 2 commits December 26, 2015 18:21

beta function implemented

6930c1b

minor fix

63f6f6e

certik reviewed Dec 26, 2015
View reviewed changes

Comment thread symengine/type_codes.inc Outdated

certik Dec 26, 2015

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

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

Space after comma.

isuruf reviewed Dec 26, 2015
View reviewed changes

sumith1896 reviewed Dec 27, 2015
View reviewed changes

Comment thread symengine/functions.cpp Outdated

sumith1896 Dec 27, 2015

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

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

Do you intent on both being return false ?

abhinavagarwal07 added 2 commits December 27, 2015 17:13

some fix

2408d3b

added tests for beta function

45a7503

whitespace fix

01e3c9c

certik reviewed Dec 31, 2015
View reviewed changes

Comment thread symengine/eval_arb.cpp Outdated

certik Dec 31, 2015

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

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

Space after Beta.

abhinavagarwal07 added 2 commits December 31, 2015 22:00

Whitespace fix

7cb2979

use gamma_positive_int and gamma_multiple_2 in gamma

7aab124

sumith1896 reviewed Jan 2, 2016
View reviewed changes

Comment thread symengine/functions.cpp Outdated

sumith1896 Jan 2, 2016

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

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

for ( ... ) {

isuruf reviewed Jan 2, 2016
View reviewed changes

abhinavagarwal07 added 2 commits January 3, 2016 21:13

Merge remote-tracking branch 'upstream/master' into beta-function

7b72fb2

some fix

0da24f3

some fix in beta function and tests

cfb5baa

isuruf reviewed Jan 4, 2016
View reviewed changes

abhinavagarwal07 and others added 2 commits January 5, 2016 18:36

added from_two_basics

e62e0ef

Merge pull request #6 from abhinavagarwal07/temp

e8c93a6

add Beta::from_two_basic

abhinavagarwal07 added 2 commits January 6, 2016 06:38

Merge remote-tracking branch 'upstream/master' into beta-function

893fec4

add beta in derivative.cpp

2eef55d

whitespace fix

c2738bf

seems like trailing whitespace check utility has failed as tests before this commits have passed

isuruf reviewed Jan 6, 2016
View reviewed changes

Comment thread symengine/functions.cpp

isuruf Jan 6, 2016

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

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

You don't need these lines anymore as the terms are ordered

abhinavagarwal07 mentioned this pull request Jan 7, 2016

Beta function #747

Merged

abhinavagarwal07 closed this Jan 7, 2016

abhinavagarwal07 deleted the beta-function branch January 8, 2016 16:36

Conversation

abhinavagarwal07 commented Dec 26, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

certik commented Dec 26, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhinavagarwal07 commented Dec 26, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhinavagarwal07 commented Dec 27, 2015

Uh oh!

sumith1896 commented Dec 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhinavagarwal07 commented Jan 1, 2016

Uh oh!

abhinavagarwal07 commented Jan 2, 2016

Uh oh!

sumith1896 commented Jan 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sumith1896 commented Jan 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

certik commented Jan 3, 2016

Uh oh!

isuruf commented Jan 3, 2016

Uh oh!

abhinavagarwal07 commented Jan 3, 2016

Uh oh!

isuruf commented Jan 3, 2016

Uh oh!

abhinavagarwal07 commented Jan 3, 2016

Uh oh!

abhinavagarwal07 commented Jan 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

certik commented Jan 5, 2016

Uh oh!

abhinavagarwal07 commented Jan 5, 2016

Uh oh!

certik commented Jan 5, 2016

Uh oh!

abhinavagarwal07 commented Jan 6, 2016

Uh oh!

certik commented Jan 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhinavagarwal07 commented Jan 7, 2016

Uh oh!

abhinavagarwal07 commented Jan 7, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants