Real Double Class #46

Merged
merged 11 commits into from Jun 1, 2016

Projects

None yet

5 participants

@rajithv
Contributor
rajithv commented May 28, 2016 edited

I have several points to clear up

I used sympify rather than initialize (same as in Complex and Rational classes). But with this the Integer class seems to be the only one different from the rest. Is it okay if we go ahead and change the integer class to be constructed throught sympify.
i.e. rather than A = SymEngine::Integer.new(5) we can use A = SymEngine(5).
Also, the macro GET_SYMINTFROMVAL(num_value, this) can be incorporated into sympify method in symengine_utils.c.

Please advice how to proceed. @abinashmeher999 @isuruf

P.S. Also, all the commits from the other PR came into this as well.. will this be an issue ?

rajithv added some commits May 28, 2016
@rajithv rajithv Real Dobule 4e26bfd
@rajithv rajithv Memory Leak fixed
a117bae
@rajithv rajithv changed the title from Real Double Class to [WIP] Real Double Class May 28, 2016
@rajithv
Contributor
rajithv commented May 28, 2016

Another uncertainity, as per the following code:

2.1.8 :005 > c4 = 4.5 + (7i/9)
 => (4.5+(7/9)*i) 
2.1.8 :006 > c5 = SymEngine(c4)
 => #<SymEngine::ComplexDouble(4.5 + 0.777777777777778*I)>

What should we do with this kind of complex numbers?

@isuruf
Member
isuruf commented May 28, 2016

That's fine. It was a design decision of symengine to keep it like that. Here, real part is machine precision, while imaginary part is infinite precision. Most often you don't want to have the real part in one precision and the imaginary part in another.

@rajithv rajithv Spec for Real Double
940a144
@isuruf
Member
isuruf commented May 29, 2016

Can you merge with master?

rajithv added some commits May 29, 2016
@rajithv rajithv Merge branch 'master' of http://github.com/symengine/symengine.rb int…
…o double2
5a58b37
@rajithv rajithv to_f and to_c methods
0e4bfd4
@isuruf isuruf and 1 other commented on an outdated diff May 29, 2016
ext/symengine/symengine_utils.c
switch(TYPE(operand2)) {
case T_FIXNUM:
case T_BIGNUM:
GET_SYMINTFROMVAL(operand2, cbasic_operand2);
break;
+ case T_FLOAT:
+ Rb_Temp_String = rb_funcall(operand2, rb_intern("to_s"), 0, NULL);
+
+ s = StringValueCStr(Rb_Temp_String);
@isuruf
isuruf May 29, 2016 Member

Use rb_float_value. Don't use strings unless you really have to.

@abinashmeher999
abinashmeher999 May 29, 2016 Contributor

Yes, that will be much faster than this.

@abinashmeher999
Contributor

@rajithv You should first understand why were the initialize methods removed for Rational and Complex classes. Integers are a subset of Rationals, Rationals of Real and Real of Complex. So when a complex is initialised with imaginary part as 0, it should fall down to Real or Rational. Likewise when a Rational is initialised with 1 as denominator it should fall down to Integer. So it made no sense to output an Integer when .new was being called for a Rational number. But is it so with Integer?

For SymEngine(5), it should still give an Integer. So to have this, .new for Integer need not be removed.

I don't see the reason to remove GET_SYMINTFROMVAL if that is what you mean by saying "incorporating it to sympify" Someone would use that macro when (s)he is expecting an Integer from VALUE and nothing else, so that if a wrong input comes an exception is thrown.

@abinashmeher999 abinashmeher999 commented on an outdated diff May 29, 2016
lib/symengine/real_double.rb
@@ -0,0 +1,7 @@
+module SymEngine
+ class RealDouble
+ def to_f
+ self.to_s.to_f
@abinashmeher999
abinashmeher999 May 29, 2016 Contributor

Same here. You can do this in C API without having to convert it into string first.

@rajithv
Contributor
rajithv commented May 30, 2016

@abinashmeher999 Thanks for the explanation.

I meant to remove the GET_SYMINTFROMVAL macro, and have that code inside sympify under the

case T_FIXNUM:
case T_BIGNUM:

then someone expecting a Integer from a VALUE can simply use simpify, with and if they expect nothing else use an assertion on that side. Or we can have a method, which calls sympify and raises an exception if sympify returns a non-SymEngine::Integer. This is my suggestion. What do you think?

@isuruf
Member
isuruf commented May 30, 2016

I think the current way is fine. You can change GET_SYMINTFROMVAL macro to a function. See #47

@abinashmeher999
Contributor

That can also be done. Think about the cases when the user doesn't want anything other than Integer. You do some computation to sympify it to some other Class only to be rejected later, and that much computation is wasted.

If you could somehow bring the selection part to before the computation is done to convert it then it would be great.

@rajithv rajithv referenced this pull request in symengine/symengine May 30, 2016
Merged

Implementing real_double_get_d method #966

rajithv added some commits May 30, 2016
@rajithv rajithv Changing to_f method for real_double
46ee731
@rajithv rajithv Fixes #47
3432fed
@rajithv rajithv Complex Double real_part, imaginary_part and spec
7c170a5
@rajithv rajithv Update symengine version
8d07eab
@rajithv
Contributor
rajithv commented May 31, 2016

The rb_float_value failes on Ruby 1.9.3 .. it is not in ruby versions prior to 2.

see : Ruby 1.9.3 vs Ruby 2.0.0

Is there a way to do version dependent coding for the Ruby C API?

@isuruf
Member
isuruf commented May 31, 2016

Use RFLOAT_VALUE

@rajithv rajithv used RFLOAT_VALUE to fix error with Ruby 1.9.3
681ee2e
@rajithv rajithv changed the title from [WIP] Real Double Class to Real Double Class May 31, 2016
@v0dro
v0dro commented May 31, 2016

Looks good. What will be the next class you plan to expose through Ruby?

@zverok
Collaborator
zverok commented May 31, 2016

(BTW, I'm not sure somebody should support 1.9.3 in a new Ruby gems in 2016, as even Ruby 2.0 is not officially supported anymore.)

@isuruf isuruf merged commit f3d3821 into symengine:master Jun 1, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment