Opened 4 years ago

Closed 17 months ago

#20368 closed enhancement (fixed)

Squarefree part

Reported by: mmezzarobba Owned by:
Priority: major Milestone: sage-8.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 vdelecroix)

Generic radical and squarefree_part in the category of unique factorization domain.

Change History (17)

comment:1 Changed 4 years ago by mmezzarobba

  • Authors set to Marc Mezzarobba
  • Branch set to u/mmezzarobba/20368-squarefree_part
  • Cc mmarco added
  • Commit set to 4a7a793f93ae3f9cb18090643dae86376b116363
  • Component changed from PLEASE CHANGE to algebra
  • Status changed from new to needs_review

New commits:

4a7a793add a generic squarefee_part()

comment:2 Changed 4 years ago by git

  • Commit changed from 4a7a793f93ae3f9cb18090643dae86376b116363 to 0244cd2362e95f87ff3c5e47085d50f68664b095

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

0244cd2add a generic squarefee_part()

comment:3 follow-up: Changed 4 years ago by jmantysalo

  • 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 ; follow-up: Changed 4 years ago by mmezzarobba

  • 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 ; follow-up: Changed 4 years ago by 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.

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 git

  • Commit changed from 0244cd2362e95f87ff3c5e47085d50f68664b095 to fa03f1cbe61eb595c9179aa76527fa690535250e

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

fa03f1cadd generic impls of squarefee_part() and radical()

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

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!

Last edited 4 years ago by mmezzarobba (previous) (diff)

comment:8 Changed 4 years ago by jdemeyer

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 git

  • Commit changed from fa03f1cbe61eb595c9179aa76527fa690535250e to fefc07b9c021224e321b30a67cab7200149dcbaa

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

8474ea3Fix up 75c75e9 (#24285)
fefc07badd generic impls of squarefee_part() and radical()

comment:10 in reply to: ↑ 7 Changed 21 months ago by mmezzarobba

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 generic squarefree_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() in arith/misc.py and probably remove the implementation using factor() from that.

Thanks for the suggestion. I moved the implementation using factor() to my new method.

comment:11 Changed 21 months ago by git

  • Commit changed from fefc07b9c021224e321b30a67cab7200149dcbaa to 9b8c8af4753a6eb9a5342e3eb80aed0ba622b9fa

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

9b8c8afadd generic impls of squarefee_part() and radical()

comment:12 Changed 21 months ago by git

  • Commit changed from 9b8c8af4753a6eb9a5342e3eb80aed0ba622b9fa to 929c5d08d5a10b0ced1d56f7aa460a49815cbb05

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

929c5d0add generic impls of squarefee_part() and radical()

comment:13 Changed 21 months ago by mmezzarobba

  • Status changed from needs_work to needs_review

comment:14 Changed 17 months ago by git

  • Commit changed from 929c5d08d5a10b0ced1d56f7aa460a49815cbb05 to e8340ac237b13106bc47df5102c08253634f28fe

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

e8340acadd generic impls of squarefee_part() and radical()

comment:15 Changed 17 months ago by vdelecroix

  • Description modified (diff)
  • Milestone changed from sage-7.2 to sage-8.3
  • Reviewers set to Vincent Delecroix
  • Status changed from needs_review to positive_review

comment:16 Changed 17 months ago by mmezzarobba

Thanks!

comment:17 Changed 17 months ago by vbraun

  • Branch changed from u/mmezzarobba/20368-squarefree_part to e8340ac237b13106bc47df5102c08253634f28fe
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.