Opened 4 years ago

Closed 4 years ago

#17624 closed enhancement (fixed)

Convert factorization to symbolic expression

Reported by: gagern Owned by:
Priority: major Milestone: sage-6.10
Component: symbolics Keywords:
Cc: Merged in:
Authors: Ralf Stephan Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 9c461e3 (Commits) Commit: 9c461e3f14d5d48fffc2bb59ee17e37a8afff125
Dependencies: Stopgaps:

Description

Dealing with polynomial rings is nice, but the fact that they always present their content in fully expanded form can make it hard to read results. For a single polynomial, computing its factorization can help. But for e.g. a vector or matrix composed of factorizations, this is not possible.

sage: R.<a,b> = QQ[]
sage: m = matrix([[a^2-b^2, a^3-a*b^2], [a*b + b^2, -77*a+77*b]])
sage: factor(m[0,0])
(a - b) * (a + b)
sage: m.apply_map(factor)
TypeError: x must be a list
sage: SR(factor(m[0,0]))
TypeError: 
sage: m.apply_map(lambda x: SR(str(factor(x))))
[  (a + b)*(a - b) (a + b)*(a - b)*a]
[        (a + b)*b      -77*a + 77*b]
sage: m[1,1].factor()
(-77) * (a - b)

It would be nice if the apply_map(factor) call would simply work, returning a matrix of symbolic expressions to help reading that matrix. The detour via str is a very hackish workaround, in my opinion.

Note that SR will automatically expand some things, as evidenced by the coeffcicient 77 in the example above. I guess that could only be avoided if we could have matrices of factorizations. That would be a viable alternative, and might be worth its own ticket, but I think coercion to SR might have other benefits as well, so this ticket here is about that coercion.

Change History (21)

comment:1 Changed 4 years ago by rws

  • Branch set to u/rws/coerce_factorization_of_polynomial_to_symbolic_expression

comment:2 Changed 4 years ago by rws

  • Authors set to Ralf Stephan
  • Commit set to ec9cd10617f78bb1a8a0d1550d1accb6c765bc1e
  • Milestone changed from sage-6.5 to sage-6.10
  • Status changed from new to needs_review

New commits:

ec9cd1017624: coerce factorizations to SR

comment:3 Changed 4 years ago by bruno

While the coercion seems to work as expected, it does not solve the "apply_map" issue. What else is needed for this?

sage: R.<a,b> = QQ[]
sage: m = matrix([[a^2-b^2, a^3-a*b^2], [a*b + b^2, -77*a+77*b]])
sage: m.apply_map(factor)
Traceback (most recent call last):
...
TypeError: x must be a list

comment:4 Changed 4 years ago by rws

Well it doesn't work with integers, either:

sage: m = matrix([[6,15], [35,49]])
sage: m.apply_map(factor)
...

and so is clearly not a problem of this ticket.

Of course you need to explicitly cast to SR (like you did in the ticket description):

sage: R.<a,b> = QQ[]
sage: m = matrix([[a^2-b^2, a^3-a*b^2], [a*b + b^2, -77*a+77*b]])
sage: f = lambda n: SR(factor(n))
sage: m.apply_map(f)
[   (a + b)*(a - b) -(a + b)*(a - b)*a]
[         (a + b)*b              a - b]

It's just that the string crutch is no longer needed.

comment:5 Changed 4 years ago by rws

  • Status changed from needs_review to needs_work

OTOH I just found a bug (77?).

comment:6 Changed 4 years ago by git

  • Commit changed from ec9cd10617f78bb1a8a0d1550d1accb6c765bc1e to d9f2c454bab76af892452cfefbf4b62790feb6c5

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

d9f2c4517624: fix previous commit

comment:7 Changed 4 years ago by rws

  • Status changed from needs_work to needs_review

comment:8 follow-up: Changed 4 years ago by nbruin

It seems that the following, using SR's "factor", works pretty well for the problem in the question:

sage: factor(SR(m))
[  (a + b)*(a - b) (a + b)*(a - b)*a]
[        (a + b)*b      -77*a + 77*b]

The problem with doing it the other way around is that the intermediate result would have to be a matrix with elements coming from ... Factorization? While we could have that, it would require a significant design and refactoring effort. Do we have a good reason to do that?

With factorizations themselves convertible to SR (which is quite reasonable and easy to do, as rws shows) one can do:

matrix(SR,m.nrows(),m.ncols(),[SR(factor(c)) for c in m.list()])

which is not hacky, albeit a bit verbose. It's basically "apply_map" spelled out.

comment:9 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work
  • Summary changed from Coerce factorization of polynomial to symbolic expression to Convert factorization to symbolic expression

I think it's safer to explicitly convert the unit also to SR.

comment:10 Changed 4 years ago by git

  • Commit changed from d9f2c454bab76af892452cfefbf4b62790feb6c5 to 23b24efe0f7355f312b5b38f9cf8d520240903a3

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

23b24ef17624: convert factorization unit to SR

comment:11 follow-up: Changed 4 years ago by rws

  • Status changed from needs_work to needs_review

I thought that would be done by giving it second for multiplication?

comment:12 in reply to: ↑ 11 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Replying to rws:

I thought that would be done by giving it second for multiplication?

Well, I think it's better to be explicit. But your comment inspired me that you should do this instead:

return prod([SR(p)**e for p,e in x], SR(x.unit()))

comment:13 Changed 4 years ago by git

  • Commit changed from 23b24efe0f7355f312b5b38f9cf8d520240903a3 to 9d1432aa35cf5005a6431c0911b2e96b0de615f7

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

9d1432a17426: cosmetics

comment:14 Changed 4 years ago by rws

  • Status changed from needs_work to needs_review

So, assuming SR.unit==1 is safe then...

comment:15 Changed 4 years ago by git

  • Commit changed from 9d1432aa35cf5005a6431c0911b2e96b0de615f7 to 9c461e3f14d5d48fffc2bb59ee17e37a8afff125

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

9c461e317426: fix typo

comment:16 in reply to: ↑ 8 ; follow-up: Changed 4 years ago by mmezzarobba

Replying to nbruin:

The problem with doing it the other way around is that the intermediate result would have to be a matrix with elements coming from ... Factorization? While we could have that, it would require a significant design and refactoring effort.

Is there any real obstacle to making Factorizations Elements of their universe()?

comment:17 in reply to: ↑ 16 ; follow-up: Changed 4 years ago by jdemeyer

Replying to mmezzarobba:

Is there any real obstacle to making Factorizations Elements of their universe()?

But that parent couldn't possibly be a ring (what's the sum of 2 factorisations?), so you wouldn't be able to make matrices over it.

comment:18 in reply to: ↑ 17 ; follow-up: Changed 4 years ago by mmezzarobba

Replying to jdemeyer:

Replying to mmezzarobba:

Is there any real obstacle to making Factorizations Elements of their universe()?

But that parent couldn't possibly be a ring (what's the sum of 2 factorisations?), so you wouldn't be able to make matrices over it.

When the universe is a ring, + could always return a “normal” element. In other words: what I'm suggesting is not to have “Factorizations over Foo” as a new parent, but to make Factorizations over Foo (an alternative representation of) elements of Foo.

comment:19 in reply to: ↑ 18 Changed 4 years ago by jdemeyer

Replying to mmezzarobba:

make Factorizations over Foo (an alternative representation of) elements of Foo.

I don't think that this will work properly. Within the current Parent/Element framework, it would require that FactorizationOfFoo is a subclass of Foo. Since Foo is usually a Cython class, it must be the first base class. Unless you want to play metaclass games, this is the wrong __mro__.

comment:20 Changed 4 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:21 Changed 4 years ago by vbraun

  • Branch changed from u/rws/coerce_factorization_of_polynomial_to_symbolic_expression to 9c461e3f14d5d48fffc2bb59ee17e37a8afff125
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.