Opened 5 years ago

Closed 5 years ago

#23525 closed enhancement (fixed)

implement is_squarefree for p-adic rings

Reported by: Julian Rüth Owned by:
Priority: minor Milestone: sage-8.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:

Status badges

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 Julian Rüth

Branch: u/saraedum/implement_is_squarefree_for_p_adic_rings

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

Authors: Julian Rüth
Cc: David Roe added
Commit: 8051942732eb094eca775f2a6ad91b361c3c5e1f
Status: newneeds_review

I did not run any doctests yet (still rebuilding sage.)


New commits:

8051942is_squarefree() for p-adics

comment:3 Changed 5 years ago by David Roe

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 Julian Rüth

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 Julian Rüth

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 git

Commit: 8051942732eb094eca775f2a6ad91b361c3c5e1f3875877441a7715c8ee032ff3dd36f625c57f6ac

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

3875877simplify field case

comment:7 Changed 5 years ago by David Roe

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 David Roe

Reviewers: David Roe
Status: needs_reviewpositive_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 Volker Braun

Branch: u/saraedum/implement_is_squarefree_for_p_adic_rings3875877441a7715c8ee032ff3dd36f625c57f6ac
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.