Opened 2 years ago

Closed 2 years ago

#23525 closed enhancement (fixed)

implement is_squarefree for p-adic rings

Reported by: saraedum Owned by:
Priority: minor Milestone: sage-8.1
Component: padics Keywords:
Cc: roed Merged in:
Authors: Julian Rüth Reviewers: David Roe
Report Upstream: N/A Work issues:
Branch: 3875877 (Commits) 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 2 years ago by saraedum

  • Branch set to u/saraedum/implement_is_squarefree_for_p_adic_rings

comment:2 Changed 2 years ago by saraedum

  • Authors set to Julian Rüth
  • Cc roed added
  • Commit set to 8051942732eb094eca775f2a6ad91b361c3c5e1f
  • Status changed from new to needs_review

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


New commits:

8051942is_squarefree() for p-adics

comment:3 Changed 2 years ago by roed

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 2 years ago by saraedum

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 2 years ago by saraedum

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 2 years ago by git

  • Commit changed from 8051942732eb094eca775f2a6ad91b361c3c5e1f to 3875877441a7715c8ee032ff3dd36f625c57f6ac

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

3875877simplify field case

comment:7 Changed 2 years ago by roed

Okay, we had some more discussion on IRC and I'm fine with this now. Positive review once tests pass.

comment:8 Changed 2 years ago by roed

  • 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 2 years ago by vbraun

  • 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
Note: See TracTickets for help on using tickets.