Opened 5 years ago
Closed 5 years ago
#23525 closed enhancement (fixed)
implement is_squarefree for padic rings
Reported by:  Julian Rüth  Owned by:  

Priority:  minor  Milestone:  sage8.1 
Component:  padics  Keywords:  
Cc:  David Roe  Merged in:  
Authors:  Julian Rüth  Reviewers:  David Roe 
Report Upstream:  N/A  Work issues:  
Branch:  3875877 (Commits, GitHub, GitLab)  Commit:  3875877441a7715c8ee032ff3dd36f625c57f6ac 
Dependencies:  Stopgaps: 
Description
Currently, this does not work
sage: R.<a> = ZqFM(9) sage: a.is_squarefree() AttributeError
Change History (9)
comment:1 Changed 5 years ago by
Branch:  → u/saraedum/implement_is_squarefree_for_p_adic_rings 

comment:2 Changed 5 years ago by
Authors:  → Julian Rüth 

Cc:  David Roe added 
Commit:  → 8051942732eb094eca775f2a6ad91b361c3c5e1f 
Status:  new → needs_review 
comment:3 Changed 5 years ago by
Maybe we should just raise a TypeError
for fields? QQ
and number fields don't define an is_squarefree
method (though elements of orders don't either). This may be another case where we can return a technically correct answer but it's most likely not what the user was expecting.
comment:4 Changed 5 years ago by
I think it makes sense to return mathematically correct answers even if they are trivial. It means that you can write algorithms without checking for trivial cases. E.g., we do this already to determine whether a polynomial is squarefree: first we check whether it's content is squarefree. A trivial check over fields, but still the correct thing to do. So currently we need to protect this check with a self.base_ring() in Fields()
to work around the fact that Fields.ElementMethods
does not implement is_squarefree()
[which it imho should.]
comment:5 Changed 5 years ago by
PS: I think it really depends on who are you writing code for. Beginners will be surprised that
sage: x = 8/2; x 4 sage: x.is_squarefree() True
but eventually you are going to learn that parents are very important in Sage…
comment:6 Changed 5 years ago by
Commit:  8051942732eb094eca775f2a6ad91b361c3c5e1f → 3875877441a7715c8ee032ff3dd36f625c57f6ac 

Branch pushed to git repo; I updated commit sha1. New commits:
3875877  simplify field case

comment:7 Changed 5 years ago by
Okay, we had some more discussion on IRC and I'm fine with this now. Positive review once tests pass.
comment:8 Changed 5 years ago by
Reviewers:  → David Roe 

Status:  needs_review → positive_review 
The only failure is the intermittent
sage t long src/sage/rings/fraction_field_FpT.pyx # 3 doctests failed
which is already addressed by #23554.
comment:9 Changed 5 years ago by
Branch:  u/saraedum/implement_is_squarefree_for_p_adic_rings → 3875877441a7715c8ee032ff3dd36f625c57f6ac 

Resolution:  → fixed 
Status:  positive_review → closed 
I did not run any doctests yet (still rebuilding sage.)
New commits:
is_squarefree() for padics