Additions to symengine/sympy_compat.py #140

ShikharJ · 2017-05-09T14:52:03Z

Relevant: #136
@isuruf The constructor for Float is itself quite complex and many of the functionalities used are currently unavailable. I have commented the specific parts as of now. What should be done here?

ShikharJ · 2017-05-11T09:22:50Z

Ping @isuruf.

isuruf · 2017-05-11T10:19:51Z

symengine/sympy_compat.py

+    _classes = (symengine.RealDouble,)
+
+    def __new__(cls, num, dps=None, prec=None, precision=None):
+        if prec is not None:


You can remove this. It's already deprecated in sympy

isuruf · 2017-05-11T10:21:19Z

What are you planning to do next?

ShikharJ · 2017-05-11T11:19:27Z

@isuruf My short term goal for the next 4-5 days would be to get this and #129 merged in. After that, I'd like to devote my attention towards porting SymEngine in sympy/parsing and sympy/physics modules (I'll continue on with this till the official coding period begins). Everything as previously planned in my proposal.

isuruf · 2017-05-11T11:22:14Z

Thanks for the update. I particularly meant what are you going to do next in this PR? Float class doesn't really do anything here

ShikharJ · 2017-05-11T11:28:37Z

That would be one of my doubts. We're currently using RealMPFR for sympy.Float objects when prec > 53. Should I try interfacing RealMPFR in place of mpmath objects (in the constructor)? Would that be a viable plan?

isuruf · 2017-05-11T11:30:22Z

Yes, Float class should return a RealDouble if the precision is <= 53 bits and a RealMPFR if not.

ShikharJ · 2017-05-13T07:32:19Z

symengine/sympy_compat.py

+                        dps = max(dps, len(str(num).lstrip('-')))    
+                    dps = max(15, dps)
+                    '''
+                    precision = mlib.libmpf.dps_to_prec(dps)


@isuruf Would you be fine with re-implementing dps_to_prec in symengine.py? Or would it cause a problem due to the licenses?

I think dps_to_prec is a one liner and that's fine

isuruf · 2017-05-13T15:29:16Z

symengine/sympy_compat.py

+                return num
+            if isinstance(num, str) and _literal_float(num):
+                try:
+                    Num = decimal.Decimal(num)


We don't need Decimal support.

isuruf · 2017-05-14T09:34:38Z

I think you are trying too much to do what sympy does.
Do something like this,

convert dps to prec
If prec <= 53 return RealDouble(str(num))
Else return RealMPFR(str(num), prec))

isuruf · 2017-05-14T12:17:38Z

symengine/sympy_compat.py

+        if dps is None and precision is None:
+            dps = 15
+            if isinstance(num, Float):
+                return num


This is wrong, what if num had a different precision?

Line 57 takes that into account. Rest cases have been accounted for later.

No, it doesn't. dps = 15 is not honoured here.

SymPy sets dps = 15 for cases where the argument is not an instance of Float. If the num is an instance of the Float class and no dps or precision is provided, it simply returns that same object. Should I change this? dps = 15 evaluates to precision = 53, so should I return a RealDouble each time?

Yes, that would be consistent. Also, you can just remove the 2 lines above and the rest of the code below should handle that case.

isuruf · 2017-05-14T12:17:52Z

symengine/sympy_compat.py

+class Float(Number):
+    _classes = (symengine.RealDouble, symengine.RealMPFR)
+
+    def __new__(cls, num, dps=None, prec=None, precision=None):


prec is not used. You can remove that

isuruf · 2017-05-14T12:18:50Z

symengine/sympy_compat.py

+        elif precision == '' and dps is None or precision is None and dps == '':
+            if not isinstance(num, (str, unicode)):   
+                raise ValueError('The null string can only be used when '
+                'the number to Float is passed as a string or an integer.')


I don't get what the purpose here is.

isuruf · 2017-05-14T12:19:21Z

symengine/sympy_compat.py

+            if not isinstance(num, (str, unicode)):   
+                raise ValueError('The null string can only be used when '
+                'the number to Float is passed as a string or an integer.')
+        if precision is None or precision == '':


Remove precision == '' part. It's unnecessary

isuruf · 2017-05-14T12:19:46Z

symengine/sympy_compat.py

+            else:
+                return RealDouble(float(str(num)))
+        else:
+            RealDouble(float(str(num)))


This should throw an error if precision > 53

isuruf · 2017-05-14T12:20:03Z

symengine/sympy_compat.py

+            if precision > 53:
+                return RealMPFR(str(num), precision)    
+            else:
+                return RealDouble(float(str(num)))


This can be RealDouble(float(num)), sorry about that

isuruf · 2017-05-15T03:18:50Z

symengine/sympy_compat.py

+            else:
+                if isinstance(num, RealDouble):
+                    return RealDouble(num.__float__())
+                elif isinstance(num, RealMPFR):


This case is impossible

isuruf · 2017-05-15T03:19:15Z

symengine/sympy_compat.py

+                    if precision == num.get_prec():
+                        return num
+                    else:
+                        return RealMPFR(str(num.__float__()), precision)


num.__float__() loses precision

isuruf · 2017-05-15T03:19:34Z

symengine/sympy_compat.py

+        if HAVE_SYMENGINE_MPFR:
+            if precision > 53:
+                if isinstance(num, RealDouble):
+                    return RealMPFR(str(num.__float__()), precision)


Remove __float__() here

isuruf · 2017-05-15T03:19:59Z

symengine/sympy_compat.py

+                if isinstance(num, RealDouble):
+                    return num
+                elif isinstance(num, RealMPFR):
+                    return RealDouble(num.__float__())


Use float(num) instead of num.__float__()

isuruf · 2017-05-15T03:20:33Z

symengine/sympy_compat.py

+                raise ValueError('RealMPFR unavailable for high precision numerical values.')
+            else:
+                if isinstance(num, RealDouble):
+                    return RealDouble(num.__float__())


Just return num here.

ShikharJ · 2017-05-15T07:35:53Z

@isuruf The builds are failing because the HAVE_SYMENGINE_FLAG is not available to sympy_compat.py and the test_sympy_compat.py. What can be done here?

isuruf · 2017-05-15T08:20:02Z

@ShikharJ, use have_mpfr in tests

isuruf · 2017-05-15T10:58:17Z

Thanks for the PR

ShikharJ · 2017-05-15T11:04:04Z

Thanks for the extensive reviewing @isuruf. You're the best!

isuruf reviewed May 11, 2017

View reviewed changes

isuruf modified the milestone: 0.3.0 May 11, 2017

ShikharJ force-pushed the SympyCompat branch from cf13a52 to f12b8c8 Compare May 13, 2017 07:10

ShikharJ commented May 13, 2017

View reviewed changes

isuruf reviewed May 13, 2017

View reviewed changes

ShikharJ force-pushed the SympyCompat branch from c1fafd3 to 85d6cc5 Compare May 14, 2017 11:39

ShikharJ changed the title ~~[WIP] Additions to symengine/sympy_compat.py~~ Additions to symengine/sympy_compat.py May 14, 2017

isuruf reviewed May 14, 2017

View reviewed changes

Added nextprime, mod_inverse and primitive_root functions

517c79a

ShikharJ force-pushed the SympyCompat branch from 85d6cc5 to f893584 Compare May 14, 2017 18:41

isuruf reviewed May 15, 2017

View reviewed changes

Added Float Class and Tests

920e433

ShikharJ force-pushed the SympyCompat branch from f893584 to 920e433 Compare May 15, 2017 07:13

Simplify Float.__new__

a22e394

Use have_mpfr in place of HAVE_SYMENGINE_MPFR

7d0c964

isuruf merged commit 6c0febf into symengine:master May 15, 2017

ShikharJ deleted the SympyCompat branch May 15, 2017 11:04

ShikharJ mentioned this pull request May 15, 2017

Add Float to symengine.sympy_compat #136

Closed

Additions to symengine/sympy_compat.py #140

Additions to symengine/sympy_compat.py #140

Uh oh!

Conversation

ShikharJ commented May 9, 2017

Uh oh!

ShikharJ commented May 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

isuruf commented May 11, 2017

Uh oh!

ShikharJ commented May 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

isuruf commented May 11, 2017

Uh oh!

ShikharJ commented May 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

isuruf commented May 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

isuruf commented May 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShikharJ May 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShikharJ commented May 15, 2017

Uh oh!

isuruf commented May 15, 2017

Uh oh!

isuruf commented May 15, 2017

Uh oh!

ShikharJ commented May 15, 2017

Uh oh!

Uh oh!

ShikharJ commented May 11, 2017 •

edited

Loading

ShikharJ commented May 11, 2017 •

edited

Loading

ShikharJ May 15, 2017 •

edited

Loading