sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.35k stars 462 forks source link

Adding a "together" function for combining sums of fractions #21477

Open c370f75d-aa64-4088-b68b-2377dcc777c5 opened 8 years ago

c370f75d-aa64-4088-b68b-2377dcc777c5 commented 8 years ago

Mathematica has a function "together" which I think is useful. https://reference.wolfram.com/language/ref/Together.html

I would like there to be something similar for Sage. As the Mathematica description states, its job is to combine fractions over a common denominator.

Component: symbolics

Issue created by migration from https://trac.sagemath.org/ticket/21477

c370f75d-aa64-4088-b68b-2377dcc777c5 commented 8 years ago

Branch: u/zonova/adding_atogetherfunction_for_combining_sums_of_fractions

c370f75d-aa64-4088-b68b-2377dcc777c5 commented 8 years ago
comment:2

I'm getting an error when trying to build sage with my changes. I simply copied placeholder comments from a different function, I'll be changing them later. All the function does is call maxima's combine function, and then also use Sage's simplify_rational function. Here is the error:

Error compiling Cython file:
------------------------------------------------------------
...
"""
Miscellaneous utilities
"""

from cpython.object cimport PyObject, Py_TYPE, descrgetfunc
^
------------------------------------------------------------

sage/structure/misc.pyx:5:0: 'cpython/object/descrgetfunc.pxd' not found

Error compiling Cython file:
------------------------------------------------------------
...
        dummy_error_message.cls = type(self)
        dummy_error_message.name = name
        raise dummy_attribute_error
    attribute = <object>attr
    # Check for a descriptor (__get__ in Python)
    cdef descrgetfunc getter = Py_TYPE(attribute).tp_descr_get
        ^
------------------------------------------------------------

sage/structure/misc.pyx:303:9: 'descrgetfunc' is not a type identifier

Error compiling Cython file:
------------------------------------------------------------
...
        dummy_error_message.cls = type(self)
        dummy_error_message.name = name
        raise dummy_attribute_error
    attribute = <object>attr
    # Check for a descriptor (__get__ in Python)
    cdef descrgetfunc getter = Py_TYPE(attribute).tp_descr_get
                                                ^
------------------------------------------------------------

sage/structure/misc.pyx:303:49: Object of type 'PyTypeObject' has no attribute 'tp_descr_get'
Traceback (most recent call last):
  File "/home/saad/sage/local/lib/python2.7/site-packages/Cython-0.24-py2.7-linux-x86_64.egg/Cython/Build/Dependencies.py", line 1052, in cythonize_one_helper
    return cythonize_one(*m)
  File "/home/saad/sage/local/lib/python2.7/site-packages/Cython-0.24-py2.7-linux-x86_64.egg/Cython/Build/Dependencies.py", line 1034, in cythonize_one
    raise CompileError(None, pyx_file)
CompileError: sage/structure/misc.pyx
[  2/219] Cythonizing sage/structure/parent.pyx
************************************************************************
Traceback (most recent call last):
  File "setup.py", line 597, in <module>
    run_cythonize()
  File "setup.py", line 589, in run_cythonize
    'profile': profile,
  File "/home/saad/sage/local/lib/python2.7/site-packages/Cython-0.24-py2.7-linux-x86_64.egg/Cython/Build/Dependencies.py", line 903, in cythonize
    result.get(99999)  # seconds
  File "/home/saad/sage/local/lib/python/multiprocessing/pool.py", line 567, in get
    raise self._value
CompileError: sage/structure/misc.pyx
************************************************************************
Error building the Sage library
************************************************************************

New commits:

0bacc83adding first test of together function
c370f75d-aa64-4088-b68b-2377dcc777c5 commented 8 years ago

Commit: 0bacc83

AndrewMathas commented 8 years ago
comment:4

I know that the name of the new method, together, has been chosen so as to be compatible with mathematica but that it would be better to chose a more meaningful name such as as_fraction or rational_function or ... Propagating poorly chosen methods names from other CASs is, I think, a mistake.

rwst commented 8 years ago
comment:5

Really? You think you can simply copy simplify_rational() without adapting its doctests? Have you tested your code by compiling it and using the resulting Sage? (You would notice that you cannot use the together command because it's not loaded into the interactive namespace) Besides it would be much easier to add the line together = simplify_rational.

c370f75d-aa64-4088-b68b-2377dcc777c5 commented 8 years ago
comment:6

Replying to @AndrewAtLarge:

I know that the name of the new method, together, has been chosen so as to be compatible with mathematica but that it would be better to chose a more meaningful name such as as_fraction or rational_function or ... Propagating poorly chosen methods names from other CASs is, I think, a mistake.

This is a good point, do you think the name combine_frac would work? There is already a simplify_rational function or else I might be tempted to use that. However, I would like all simplifying functions to begin with the word simplify, so I'm not too sure what to call it.

Replying to @rwst:

Really? You think you can simply copy simplify_rational() without adapting its doctests? Have you tested your code by compiling it and using the resulting Sage? (You would notice that you cannot use the together command because it's not loaded into the interactive namespace) Besides it would be much easier to add the line together = simplify_rational.

I'm not sure whether I'm understanding you correctly, but I want to clarify that this function isn't the same as simplify_rational. I simply copied the comment section for simplify_rational. I tried compiling it and it didn't work, it gave me the above error. I then took out the comment all together and it still didn't compile correctly. I posted the error message above and it's not pointing back to my edit as far as I can tell, it's tracing some error in cython. Not really sure what's up.

c370f75d-aa64-4088-b68b-2377dcc777c5 commented 8 years ago

Changed commit from 0bacc83 to none

c370f75d-aa64-4088-b68b-2377dcc777c5 commented 8 years ago

Changed branch from u/zonova/adding_atogetherfunction_for_combining_sums_of_fractions to none