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

is_squarefree() incorrect over imperfect fields #12404

Closed
saraedum opened this issue Jan 31, 2012 · 33 comments
Closed

is_squarefree() incorrect over imperfect fields #12404

saraedum opened this issue Jan 31, 2012 · 33 comments

Comments

@saraedum
Copy link
Member

The method is_squarefree() is incorrect for polynomials over function fields over finite fields:

sage: K.<x> = FunctionField(GF(3))
sage: R.<T> = K[]
sage: f=T^3-x
sage: f.factor(proof=False)
T^3 + 2*x
sage: f.is_squarefree()
False

Apply:

  1. attachment: trac_12404.patch
  2. attachment: trac_12404_warning.patch
  3. attachment: 12404_examples.patch

to the sage library.

Depends on #9054
Depends on #13043
Depends on #12988
Depends on #10902

CC: @sagetrac-sydahmad

Component: commutative algebra

Keywords: sd40.5

Author: Julian Rueth

Reviewer: Paul Zimmermann, Jeroen Demeyer

Merged: sage-5.1.beta3

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

@saraedum
Copy link
Member Author

comment:1

The method is_squarefree() returns self.derivative().gcd(self).degree() <= 0. In the example the derivative vanishes which leads to the incorrect behaviour.

As a quick workaround one can do:

True in [d>=2 for (f,d) in f.factor(proof=False)]

@saraedum
Copy link
Member Author

comment:2

Replying to @saraedum:

True in [d>=2 for (f,d) in f.factor(proof=False)]

This would be "is_not_squarefree()" of course.

@zimmermann6
Copy link
Contributor

comment:3

see also #12198.

Paul

@zimmermann6
Copy link
Contributor

comment:4

see also #12129.

Paul

@saraedum
Copy link
Member Author

comment:5

Thanks for pointing these out. Since both are in the multivariate case, I don't think they're related.

@saraedum
Copy link
Member Author

comment:6

The attached patch should fix the problem.

@saraedum
Copy link
Member Author

Changed dependencies from #9054 to #9054, #12404

@saraedum
Copy link
Member Author

Author: Julian Rueth

@saraedum
Copy link
Member Author

Changed dependencies from #9054, #12404 to #9054, #12988

@saraedum
Copy link
Member Author

Changed keywords from none to sd40.5

@zimmermann6
Copy link
Contributor

Work Issues: coherence with squarefree_decomposition, fix error

@zimmermann6
Copy link
Contributor

comment:9

the documentation of is_squarefree says that f is not square-free if g^2
divides f where g is a non-unit. In particular 4*x is not considered
square free by is_squarefree, but it is by squarefree_decomposition:

sage: R.<x> = ZZ[]
sage: f = 4*x
sage: f.is_squarefree()
False
sage: f.squarefree_decomposition()
(4) * x

Sage should be coherent in that matter. Personally I prefer not to decompose the coefficient content.

Moreover the following produces an error:

sage: R.<x> = ZZ[]
sage: f = 2*x^2
sage: f.is_squarefree()
...
AttributeError: 'int' object has no attribute 'is_zero'

Paul

@zimmermann6
Copy link
Contributor

Reviewer: Paul Zimmermann

@saraedum
Copy link
Member Author

comment:10

Replying to @zimmermann6:

the documentation of is_squarefree says that f is not square-free if g^2
divides f where g is a non-unit. In particular 4*x is not considered
square free by is_squarefree, but it is by squarefree_decomposition:

sage: R.<x> = ZZ[]
sage: f = 4*x
sage: f.is_squarefree()
False
sage: f.squarefree_decomposition()
(4) * x

Sage should be coherent in that matter. Personally I prefer not to decompose the coefficient content.

That is true. I also noted this inconsistency. To not break existing code, I'd rather add a warning section in the docstring. Generally I agree that having squarefree_decomposition() factor the content is not what one wants for most purposes.

Would you be ok with just adding a warning and an example showing this problem?

Moreover the following produces an error:

sage: R.<x> = ZZ[]
sage: f = 2*x^2
sage: f.is_squarefree()
...
AttributeError: 'int' object has no attribute 'is_zero'

This should not happen when applying the dependency #12988.

@saraedum
Copy link
Member Author

comment:12

I added a patch with such a warning. I still have to check how it renders in the docs.

@saraedum
Copy link
Member Author

comment:13

I made a few docstring changes in the latest patch.

@saraedum

This comment has been minimized.

@saraedum
Copy link
Member Author

Attachment: trac_12404.patch.gz

distinguish characteristic zero and nonzero

@saraedum
Copy link
Member Author

comment:14

Btw. I will add the respective warning for squarefree_decomposition() in #13008.

@zimmermann6
Copy link
Contributor

comment:15

Julian, there is a typo in trac_12404_warning.patch: not to consider to content
should read not to consider the content.

Would it break a lot of code if is_squarefree returns False for 4*x?
Did you try?

Paul

@saraedum
Copy link
Member Author

Attachment: trac_12404_warning.patch.gz

warning about inconsistency with squarefree_decomposition

@saraedum
Copy link
Member Author

comment:16

Thanks, the typo should be fixed now.

No I haven't tried. I was actually thinking about external code using that method. If you insist we can change it. I don't have a very strong opinion about this. I just believe that inconsistency isn't bad as long as it's documented. IMHO breaking the interface is worse than documented inconsistency.

@jdemeyer
Copy link
Contributor

comment:17

Replying to @zimmermann6:

Would it break a lot of code if is_squarefree returns False for 4*x?

Did you mean "if is_squarefree returns True for 4*x"?

@jdemeyer
Copy link
Contributor

Changed dependencies from #9054, #12988 to #9054, #12988, #10902

@jdemeyer
Copy link
Contributor

Attachment: 12404_examples.patch.gz

@jdemeyer
Copy link
Contributor

comment:19

Positive review to the first two patches. Anybody else can review my patch?

@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link
Contributor

Changed reviewer from Paul Zimmermann to Paul Zimmermann, Jeroen Demeyer

@williamstein
Copy link
Contributor

comment:20

jdmeyer -- I positively review your patch.

@jdemeyer
Copy link
Contributor

Changed upstream from Not yet reported upstream; Will do shortly. to none

@jdemeyer
Copy link
Contributor

Changed work issues from coherence with squarefree_decomposition, fix error to none

@jdemeyer jdemeyer added this to the sage-5.1 milestone May 28, 2012
@jdemeyer
Copy link
Contributor

Changed dependencies from #9054, #12988, #10902 to #9054,#13043, #12988, #10902

@jdemeyer
Copy link
Contributor

jdemeyer commented Jun 6, 2012

Merged: sage-5.1.beta3

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

5 participants