Skip to content
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

Extend normalize() and use it instead of Maxima in simplify_rational() #21335

Open
rwst opened this issue Aug 25, 2016 · 22 comments
Open

Extend normalize() and use it instead of Maxima in simplify_rational() #21335

rwst opened this issue Aug 25, 2016 · 22 comments

Comments

@rwst
Copy link

rwst commented Aug 25, 2016

At the moment normalize() will not expand factors of fractions. If fractions are combined however, the final numerator is expanded:

sage: ((x^(y/2) + 1)^2*(x^(y/2) - 1)^2/(x^y - 1)).normalize()
(x^(1/2*y) + 1)^2*(x^(1/2*y) - 1)^2/(x^y - 1)
sage: (x + y^2/(x + 2)).normalize()
(x^2 + y^2 + 2*x)/(x + 2)

The alternatives are provided atm by Maxima

sage: ((x^(y/2) + 1)^2*(x^(y/2) - 1)^2/(x^y - 1)).simplify_rational(algorithm='simple')
(x^(2*y) - 2*x^y + 1)/(x^y - 1)
sage: (x + y^2/(x + 2)).simplify_rational(algorithm='noexpand')
((x + 2)*x + y^2)/(x + 2)

pynac/pynac#191

After Pynac will have implemented normalize() options to these effect the calls to Maxima in simplify_rational should be replaced by the respective calls to Pynac.

Depends on #21369
Depends on #21529

Component: symbolics

Author: Ralf Stephan

Branch/Commit: u/rws/noexpand_simple_options_for_ex_normalize__ @ 30bc79e

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

@rwst rwst added this to the sage-7.4 milestone Aug 25, 2016
@rwst

This comment has been minimized.

@rwst
Copy link
Author

rwst commented Aug 29, 2016

comment:2

This is implemented in Pynac master but one doctest depends on #21360. Also, nested application ("full") needs to be done.

@rwst
Copy link
Author

rwst commented Aug 29, 2016

Dependencies: #21360

@rwst
Copy link
Author

rwst commented Aug 29, 2016

Changed dependencies from #21360 to none

@rwst
Copy link
Author

rwst commented Aug 31, 2016

@rwst
Copy link
Author

rwst commented Aug 31, 2016

New commits:

b996f4fversion/chksum
91a08d2changes affecting Sage behaviour or interface
a4f58d7doctest fixes
ac7d971revert removal of pos.char. doctests; add "known bug"
3dd8058address reviewer's comments
1f29305add doctest
0065f62address reviewer's comments
129b6e6use normal() instead of Maxima

@rwst
Copy link
Author

rwst commented Aug 31, 2016

Dependencies: #21369

@rwst
Copy link
Author

rwst commented Aug 31, 2016

Author: Ralf Stephan

@rwst
Copy link
Author

rwst commented Aug 31, 2016

Commit: 129b6e6

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 2, 2016

Branch pushed to git repo; I updated commit sha1. New commits:

b06cfa0Fix _eval_self(float) for "real" complex expressions
fd9fa19Merge branch 'u/jdemeyer/update_to_pynac_0_6_9' of trac.sagemath.org:sage into t/21335/noexpand_simple_options_for_ex_normalize__
c69fd2e21335: fullratsimp: replace calls to Maxima with Pynac ones

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 2, 2016

Changed commit from 129b6e6 to c69fd2e

@rwst
Copy link
Author

rwst commented Sep 18, 2016

comment:7

Three doctest fails in symbolic/expression.pyx.

@rwst rwst changed the title Noexpand/simple options for ex.normalize() Extend normalize() and use it instead of Maxima in simplify_rational() Sep 18, 2016
@rwst
Copy link
Author

rwst commented Sep 18, 2016

Changed dependencies from #21369 to #21369, #21529

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 18, 2016

Branch pushed to git repo; I updated commit sha1. New commits:

a8eba31Merge branch 'develop' into t/21335/noexpand_simple_options_for_ex_normalize__
2ea5c3121529: fix bug in factoring of general symbolic expressions
c4932f8Merge branch 'u/rws/bug_in_factoring_of_general_symbolic_expressions' of trac.sagemath.org:sage into t/21335/noexpand_simple_options_for_ex_normalize__

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 18, 2016

Changed commit from c69fd2e to c4932f8

@mezzarobba
Copy link
Member

comment:12

Minor point: "and and" should be "and".

Apart from that, I don't understand why algorithm="full" attempts to factor the simplified fraction (which is a potentially costly operation). Is there something in the way the Pynac normal() function works that makes it necessary to do so?

@rwst
Copy link
Author

rwst commented Oct 5, 2016

comment:13

Indeed the performance question was the source of some headaches to me, as well. I figured at the time consistency with Maxima behaviour was more important. Without factor the following doctests will fail:

File "src/sage/symbolic/expression.pyx", line 9427, in sage.symbolic.expression.Expression.simplify_rational
Failed example:
    f.simplify_rational()
Expected:
    -2*sqrt(x - 1)/sqrt((x + 1)*(x - 1))
Got:
    ((x - 1)^(3/2) - sqrt(x - 1)*x - sqrt(x - 1))/sqrt((x + 1)*(x - 1))
**********************************************************************
File "src/sage/symbolic/expression.pyx", line 9451, in sage.symbolic.expression.Expression.simplify_rational
Failed example:
    g.simplify_rational()
Expected:
    x^y - 1
Got:
    (x^(2*y) - 2*x^y + 1)/(x^y - 1)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 5, 2016

Branch pushed to git repo; I updated commit sha1. New commits:

ab551beMerge branch 'develop' into t/21335/noexpand_simple_options_for_ex_normalize__
30bc79ecosmetics

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 5, 2016

Changed commit from c4932f8 to 30bc79e

@mezzarobba
Copy link
Member

comment:15

Replying to @rwst:

Indeed the performance question was the source of some headaches to me, as well. I figured at the time consistency with Maxima behaviour was more important.

I don't know exactly what either Maxima or Pynac does, but just by chance, do you think it would be okay to keep the loop but expand the denominators instead of calling factor()?

@rwst
Copy link
Author

rwst commented Oct 5, 2016

comment:16

Replying to @mezzarobba:

I don't know exactly what either Maxima or Pynac does, but just by chance, do you think it would be okay to keep the loop but expand the denominators instead of calling factor()?

This will, additionally to the two doctests above, fail this doctest:

File "src/sage/symbolic/expression.pyx", line 9458, in sage.symbolic.expression.Expression.simplify_rational
Failed example:
    f.simplify_rational()
Expected:
    (2*x^2 + 5*x + 4)/((x + 2)^2*(x + 1))
Got:
    (2*x^2 + 5*x + 4)/(x^3 + 5*x^2 + 8*x + 4)

@rwst
Copy link
Author

rwst commented Nov 28, 2016

comment:17

I think I see now how to resolve the three tests without factoring. Pynac's gcd needs to be a bit more aware of powers in the expression and needs to replace some of them with new symbols. This way also exponentials can be handled identically (which does not work atm).

@mkoeppe mkoeppe removed this from the sage-7.4 milestone Dec 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants