Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement is_squarefree for p-adic rings #23525

Closed
saraedum opened this issue Jul 23, 2017 · 13 comments
Closed

implement is_squarefree for p-adic rings #23525

saraedum opened this issue Jul 23, 2017 · 13 comments

Comments

@saraedum
Copy link
Member

Currently, this does not work

sage: R.<a> = ZqFM(9)
sage: a.is_squarefree()
AttributeError

CC: @roed314

Component: padics

Author: Julian Rüth

Branch/Commit: 3875877

Reviewer: David Roe

Issue created by migration from https://trac.sagemath.org/ticket/23525

@saraedum saraedum added this to the sage-8.1 milestone Jul 23, 2017
@saraedum
Copy link
Member Author

@saraedum
Copy link
Member Author

comment:2

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


New commits:

8051942is_squarefree() for p-adics

@saraedum
Copy link
Member Author

Author: Julian Rüth

@saraedum
Copy link
Member Author

Commit: 8051942

@roed314
Copy link
Contributor

roed314 commented Jul 28, 2017

comment:3

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.

@saraedum
Copy link
Member Author

comment:4

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.]

@saraedum
Copy link
Member Author

comment:5

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…

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 3, 2017

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

3875877simplify field case

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 3, 2017

Changed commit from 8051942 to 3875877

@roed314
Copy link
Contributor

roed314 commented Aug 3, 2017

comment:7

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

@roed314
Copy link
Contributor

roed314 commented Aug 4, 2017

comment:8

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.

@roed314
Copy link
Contributor

roed314 commented Aug 4, 2017

Reviewer: David Roe

@vbraun
Copy link
Member

vbraun commented Aug 5, 2017

Changed branch from u/saraedum/implement_is_squarefree_for_p_adic_rings to 3875877

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants