Opened 4 years ago
Closed 17 months ago
#20368 closed enhancement (fixed)
Squarefree part
Reported by:  mmezzarobba  Owned by:  

Priority:  major  Milestone:  sage8.3 
Component:  algebra  Keywords:  
Cc:  mmarco, jmantysalo  Merged in:  
Authors:  Marc Mezzarobba  Reviewers:  Vincent Delecroix 
Report Upstream:  N/A  Work issues:  
Branch:  e8340ac (Commits)  Commit:  e8340ac237b13106bc47df5102c08253634f28fe 
Dependencies:  Stopgaps: 
Description (last modified by )
Generic radical
and squarefree_part
in the category of unique factorization domain.
Change History (17)
comment:1 Changed 4 years ago by
 Branch set to u/mmezzarobba/20368squarefree_part
 Cc mmarco added
 Commit set to 4a7a793f93ae3f9cb18090643dae86376b116363
 Component changed from PLEASE CHANGE to algebra
 Status changed from new to needs_review
comment:2 Changed 4 years ago by
 Commit changed from 4a7a793f93ae3f9cb18090643dae86376b116363 to 0244cd2362e95f87ff3c5e47085d50f68664b095
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
0244cd2  add a generic squarefee_part()

comment:3 followup: ↓ 4 Changed 4 years ago by
 Cc jmantysalo added
Could you add an example of just an integer? It would be easier to see what this is about for a beginner.
comment:4 in reply to: ↑ 3 ; followup: ↓ 5 Changed 4 years ago by
 Status changed from needs_review to needs_work
Replying to jmantysalo:
Could you add an example of just an integer? It would be easier to see what this is about for a beginner.
Argh: your comment made me realize that integers already had a squarefree_part()
method that uses a different definition of the squarefree part.
comment:5 in reply to: ↑ 4 ; followup: ↓ 7 Changed 4 years ago by
Replying to mmezzarobba:
Argh: your comment made me realize that integers already had a
squarefree_part()
method that uses a different definition of the squarefree part.
Good to notice that in this time and not later...
Can you also check #20369, if there will be something similar problem?
comment:6 Changed 4 years ago by
 Commit changed from 0244cd2362e95f87ff3c5e47085d50f68664b095 to fa03f1cbe61eb595c9179aa76527fa690535250e
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
fa03f1c  add generic impls of squarefee_part() and radical()

comment:7 in reply to: ↑ 5 ; followup: ↓ 10 Changed 4 years ago by
Replying to jmantysalo:
Replying to mmezzarobba:
Argh: your comment made me realize that integers already had a
squarefree_part()
method that uses a different definition of the squarefree part.
Okay, assuming I read the definition correctly, the two versions should now agree. Unfortunately, I don't know how to test if that's the case, since integers don't implement squarefree_decomposition()
. I guess we should implement a generic squarefree_decomposition()
, perhaps as part of this ticket, but I don't have time for that right now.
Can you also check #20369, if there will be something similar problem?
I found no mention of elementary_divisor()
anywhere in the sage source code, so I guess there won't be the same problem. Of course, there could always be another inconsistency regarding who knows what convention or definitions, like there are so many in sage!
comment:8 Changed 4 years ago by
I think that you should look at radical()
in arith/misc.py
and probably remove the implementation using factor()
from that.
comment:9 Changed 21 months ago by
 Commit changed from fa03f1cbe61eb595c9179aa76527fa690535250e to fefc07b9c021224e321b30a67cab7200149dcbaa
comment:10 in reply to: ↑ 7 Changed 21 months ago by
Replying to mmezzarobba:
Unfortunately, I don't know how to test if that's the case, since integers don't implement
squarefree_decomposition()
. I guess we should implement a genericsquarefree_decomposition()
, perhaps as part of this ticket, but I don't have time for that right now.
I changed my mind about that. The squarefree decomposition is very useful for polynomials because it can be computed efficiently, but not that interesting for, say, integers, where it requires(?) factoring. So perhaps it is actually better to leave it unimplemented until someone really needs it (as opposed to something like radical()
) for something else than polynomials.
Replying to jdemeyer:
I think that you should look at
radical()
inarith/misc.py
and probably remove the implementation usingfactor()
from that.
Thanks for the suggestion. I moved the implementation using factor()
to my new method.
comment:11 Changed 21 months ago by
 Commit changed from fefc07b9c021224e321b30a67cab7200149dcbaa to 9b8c8af4753a6eb9a5342e3eb80aed0ba622b9fa
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
9b8c8af  add generic impls of squarefee_part() and radical()

comment:12 Changed 21 months ago by
 Commit changed from 9b8c8af4753a6eb9a5342e3eb80aed0ba622b9fa to 929c5d08d5a10b0ced1d56f7aa460a49815cbb05
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
929c5d0  add generic impls of squarefee_part() and radical()

comment:13 Changed 21 months ago by
 Status changed from needs_work to needs_review
comment:14 Changed 17 months ago by
 Commit changed from 929c5d08d5a10b0ced1d56f7aa460a49815cbb05 to e8340ac237b13106bc47df5102c08253634f28fe
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
e8340ac  add generic impls of squarefee_part() and radical()

comment:15 Changed 17 months ago by
 Description modified (diff)
 Milestone changed from sage7.2 to sage8.3
 Reviewers set to Vincent Delecroix
 Status changed from needs_review to positive_review
comment:16 Changed 17 months ago by
Thanks!
comment:17 Changed 17 months ago by
 Branch changed from u/mmezzarobba/20368squarefree_part to e8340ac237b13106bc47df5102c08253634f28fe
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
add a generic squarefee_part()