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

Priority:  minor  Milestone:  sage8.1 
Component:  padics  Keywords:  
Cc:  roed  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 set to u/saraedum/implement_is_squarefree_for_p_adic_rings
comment:2 Changed 5 years ago by
 Cc roed added
 Commit set to 8051942732eb094eca775f2a6ad91b361c3c5e1f
 Status changed from new to 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 changed from 8051942732eb094eca775f2a6ad91b361c3c5e1f to 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 set to David Roe
 Status changed from needs_review to 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 changed from u/saraedum/implement_is_squarefree_for_p_adic_rings to 3875877441a7715c8ee032ff3dd36f625c57f6ac
 Resolution set to fixed
 Status changed from positive_review to closed
I did not run any doctests yet (still rebuilding sage.)
New commits:
is_squarefree() for padics