Skip to content

Add classes for Add, Mul and Pow #17

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

Merged
merged 2 commits into from
Feb 21, 2016
Merged

Conversation

rajithv
Copy link
Contributor

@rajithv rajithv commented Feb 19, 2016

Changing the class of addition, multiplication and power expressions to Add, Mul, and Pow to reflect SymEngine classes.

e = 2 * x
assert e.to_s == '2*x'
expect(e).to be_a SymEngine::Pow
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be SymEngine::Mul ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abinashmeher999 fixed this

@abinashmeher999
Copy link
Contributor

I think we should at least stick to be_an_instance_ofin the class specs because there we need to be sure that it is indeed an instance of the same class not an object where we are only enforcing the class to be somewhere in the inheritance tree. We can use be_a/be_an at the other specs like arit_spec.rb

@abinashmeher999
Copy link
Contributor

@rajithv Just a suggestion. It is good to write the tests first. That way you are sure how you want your objects to behave in Ruby and not get influenced by the C like syntax. You can use xit to allow the tests to fail till the source is written.

@rajithv
Copy link
Contributor Author

rajithv commented Feb 19, 2016

@abinashmeher999 sure.
I will do that in the future.
So I changed the checks to be_an_instance_of when the expression is required to exactly a Rational or an Integer, Add etc.

@abinashmeher999
Copy link
Contributor

Looks good to me. However, you will have to rebase once #15 is merged.

The binary and unary operators were tested in basic_spec.rb because Add, Mul and Pow weren't supported at that time. Once this PR is merged try to move those out into add_spec, mul_spec and pow_spec. Then the tests should check if the result is an instance of their respective classes.

@@ -157,7 +157,7 @@
z = SymEngine::Symbol.new('z')
e = (x**y / z)
f = e.free_symbols
expect(f).to be_an_instance_of Set
expect(f).to be_a Set
Copy link
Member

Choose a reason for hiding this comment

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

This change is unnecessary

@isuruf
Copy link
Member

isuruf commented Feb 19, 2016

+1 to merge

@certik
Copy link
Contributor

certik commented Feb 19, 2016

+1 as well. Should we wait for #15 to be merged first?

@abinashmeher999
Copy link
Contributor

We should have the tests run on this once which would be after #15.

@isuruf isuruf closed this Feb 21, 2016
@isuruf isuruf reopened this Feb 21, 2016
isuruf added a commit that referenced this pull request Feb 21, 2016
Add classes for Add, Mul and Pow
@isuruf isuruf merged commit 014a7ee into symengine:master Feb 21, 2016
@isuruf
Copy link
Member

isuruf commented Feb 21, 2016

@rajithv, thanks for the PR

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.

4 participants