sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.37k stars 462 forks source link

Convert MV polynomial constructors in multi_polynomial_ideal.py, category_object.py, etc #5332

Closed 85eec1a4-3d04-4b4d-b711-d4db03337c41 closed 9 years ago

85eec1a4-3d04-4b4d-b711-d4db03337c41 commented 15 years ago

From http://groups.google.com/group/sage-support/browse_thread/thread/21d861876918e668

> While I looked at ideal's docstring I noticed plenty of construct like 
>     sage: R, x = PolynomialRing(ZZ, 'x').objgen() 
> Shouldn't we get those cleaned up to read 
>    sage: R.<x>=ZZ[] 
> or am I missing the point? I have seen too many people use the above 
> old objgen() constuct and I find it rather hideous. 

Yeah, its just old and noone got around cleaning that up. 

Martin 

There are some more places, to find them run

grep "PolynomialRing" .doctest* | grep objgen

in $SAGE_ROOT/tmp Cheers,

Michael

CC: @malb

Component: documentation

Author: Marc Mezzarobba, Vincent Delecroix

Branch/Commit: ea55f2a

Reviewer: Vincent Delecroix, Marc Mezzarobba

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

mezzarobba commented 9 years ago

Branch: u/mmezzarobba/5332-objgen

mezzarobba commented 9 years ago

Commit: a3c87e0

mezzarobba commented 9 years ago

Author: Marc Mezzarobba

mezzarobba commented 9 years ago

New commits:

a3c87e0#5332 Modernize some examples in rings.polynomial
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

c098677#5332 Prefer R. = ... to R, x = ....objgen() in examples
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from a3c87e0 to c098677

mezzarobba commented 9 years ago
comment:9

With the last version of the patch:

$ git grep "sage: .*PolynomialRing.*objgen" src/sage/
src/sage/rings/fraction_field_element.pyx:        sage: K, x = FractionField(PolynomialRing(QQ, 'x')).objgen()
src/sage/rings/polynomial/multi_polynomial_element.py:            sage: R, x = PolynomialRing(QQbar, 10, 'x').objgens()
src/sage/rings/polynomial/multi_polynomial_ring.py:    sage: PolynomialRing(GF(5), 3, 'xyz').objgens()
src/sage/rings/polynomial/polynomial_element.pyx:    sage: PolynomialRing(ZZ,'x').objgen()
src/sage/structure/category_object.pyx:            sage: R, x = PolynomialRing(QQ,'x').objgen()
src/sage/structure/category_object.pyx:         sage: R, x = PolynomialRing(QQ,'x',12).objgens()
videlec commented 9 years ago
comment:10

This is horrible

R.<x,y> = QQ[]

(but working, I know).

Please use either

sage: R = QQ['x,y']
sage: x,y = R.gens()

or the shorter

sage: R.<x,y> = QQ['x,y']

Having something implicit in the right hand side of this declaration is really bad. Don't you think?

Vincent

mezzarobba commented 9 years ago
comment:11

Replying to @videlec:

Having something implicit in the right hand side of this declaration is really bad. Don't you think?

No :-). At least, less so that repeating the name when the intention is to have the variable name match the identifier. Also, that's both the style the ticket description suggests and a very common choice in existing sage documentation.

videlec commented 9 years ago
comment:12

Replying to @mezzarobba:

Replying to @videlec:

Having something implicit in the right hand side of this declaration is really bad. Don't you think?

No :-). At least, less so that repeating the name when the intention is to have the variable name match the identifier. Also, that's both the style the ticket description suggests and a very common choice in existing sage documentation.

This is one more thing that induces confusion between Python variables and mathematical variables (not worse than var(x) though). It is not a repetition as the semantic are very different.

bgrenet commented 9 years ago
comment:13

Replying to @videlec:

Replying to @mezzarobba:

Replying to @videlec:

Having something implicit in the right hand side of this declaration is really bad. Don't you think?

No :-). At least, less so that repeating the name when the intention is to have the variable name match the identifier. Also, that's both the style the ticket description suggests and a very common choice in existing sage documentation.

This is one more thing that induces confusion between Python variables and mathematical variables (not worse than var(x) though). It is not a repetition as the semantic are very different.

I agree with Marc, I clearly see sage: R.<x,y> = QQ['x,y'] as a repetition. And I honestly do not see the problem with sage: R.<x,y> = QQ[]: one is simply saying "I define the polynomial ring over QQ with 2 variables, and I use the Python x and y to denote its generators. I let Sage choose the way it represents graphically these generators". And then, of course it is quite nice that the default choice is to use the string "x" for x and the string "y" for y.

Otherwise stated, I like in the notation sage: R.<x,y> = QQ[] the fact that I simply define the object and not the way it is printed.

vbraun commented 9 years ago
comment:14

Its Magma syntax so its going to stay either way.

I also prefer R.<x,y> = QQ[]. Technically the variable labels also enter the right hand side, but thats just an implementation detail. We could dig the labels out of the globals dictionary without having the preparser put them into the right hand side of the declaration, so its not truly necessary. Of course the preparser implementation is superior.

kcrisman commented 9 years ago
comment:15

Its Magma syntax so its going to stay either way.

As well as being old and so liable to break others' code if removed.

+1

videlec commented 9 years ago
comment:16

All right, let me start again.

  1. I agree with all of you on what I am doing every day to implement a polynomial ring in Sage. The syntax R.<x,y> = QQ[] is compact and useful.

  2. My comment was about newcomers for which the concept of a Python variable is not necessarily clear (and the confusion with mathematic variable is automatic). So if you answer, please either be a newcomer or try to be one. Claiming as Bruno in comment:13 that the syntax is very simple is a bit of a lie.

Vincent

mezzarobba commented 9 years ago
comment:17

If the compact syntax is confusing for newcomers (which I agree it may be), I think we should better explain it in tutorials and other documentation targetted at newcomers, not avoid using it in reference documentation. So I don't see what the problem has to do with this ticket.

videlec commented 9 years ago
comment:18

Replying to @mezzarobba:

If the compact syntax is confusing for newcomers (which I agree it may be), I think we should better explain it in tutorials and other documentation targetted at newcomers, not avoid using it in reference documentation. So I don't see what the problem has to do with this ticket.

True. But the point is that sage: my_object? is also an entry point for newcomers. I am fine to have the compact syntax in ideal or elliptic_curves. But I would avoid it without explanations in polynomial_element.pyx for example.

mezzarobba commented 9 years ago
comment:19

Replying to @videlec:

True. But the point is that sage: my_object? is also an entry point for newcomers. I am fine to have the compact syntax in ideal or elliptic_curves. But I would avoid it without explanations in polynomial_element.pyx for example.

Then add a paragraph or two to explain the syntax in well-chosen docstrings...

Do you really see Polynomial.valuation? as something people who don't know how to define basic objects are likely to read?

videlec commented 9 years ago
comment:20

Replying to @mezzarobba:

Replying to @videlec:

True. But the point is that sage: my_object? is also an entry point for newcomers. I am fine to have the compact syntax in ideal or elliptic_curves. But I would avoid it without explanations in polynomial_element.pyx for example.

Then add a paragraph or two to explain the syntax in well-chosen docstrings...

Will do.

Do you really see Polynomial.valuation? as something people who don't know how to define basic objects are likely to read?

I don't know. Some researcher in algebraic geometry might.

videlec commented 9 years ago
comment:21

I will update a commit in a minute. Few questions first:

Vincent

mezzarobba commented 9 years ago
comment:22

Replying to @videlec:

I will update a commit in a minute. Few questions first:

  • what about other parts of the doc? (I have a commit ready for that)

Which “other parts” are you talking about?

  • what about the call to objgen or objgens in the code? (I have a commit to get rid of that)
  • what about deprecating the global functions objgen and objgens as well as the methods on CategoryObject?

In library code, they are very useful!

videlec commented 9 years ago
comment:23

Replying to @mezzarobba:

Replying to @videlec:

I will update a commit in a minute. Few questions first:

  • what about other parts of the doc? (I have a commit ready for that)

Which “other parts” are you talking about?

Number field

sage: K.<a> = NumberField(x^2 - 2)

Finite fields

sage: K.<a> = GF(3^5)

Power series

sage: R.<x> = QQ[[]]

Dirichlet group

sage: G.<e> = DirichletGroup(20)

There are more or less a dozen of files (including sage/schemes and sage/modular).

  • what about the call to objgen or objgens in the code? (I have a commit to get rid of that)
  • what about deprecating the global functions objgen and objgens as well as the methods on CategoryObject?

In library code, they are very useful!

I agree that the method is useful on number fields, polynomial rings, power series. But at the level of category objects or parent?

Vincent

PS: Not speaking about the fact that

K = NumberField(x^2 - 2, 'a')
a = K.gen()

is faster, less cryptic and more economic in characters than

K,a = NumberField(x^2-2,'a').objgen()
mezzarobba commented 9 years ago
comment:24

Replying to @videlec:

Which “other parts” are you talking about?

Number field

[...]

There are more or less a dozen of files (including sage/schemes and sage/modular).

Yes, why not. To be honest I made the changes here because I stumbled upon this ticket and thought it would take five minutes and make one years-old open ticket less, but I don't really care about the problem.

I agree that the method is useful on number fields, polynomial rings, power series. But at the level of category objects or parent?

Okay, I misunderstood, sorry. I don't know.

PS: Not speaking about the fact that

K = NumberField(x^2 - 2, 'a')
a = K.gen()

is faster, less cryptic and more economic in characters than

K,a = NumberField(x^2-2,'a').objgen()

I actually tend to find the latter more readable, mostly because it only takes one line.

videlec commented 9 years ago

Changed branch from u/mmezzarobba/5332-objgen to public/5332

videlec commented 9 years ago

Changed commit from c098677 to be20eb8

videlec commented 9 years ago
comment:25

Rebased on sge-6.7.beta0 with a new commit.


New commits:

0df8036#5332 Prefer R. = ... to R, x = ....objgen() in examples
be20eb8Trac 5332: remove more objgen(s) from the doc
mezzarobba commented 9 years ago
comment:26
--- a/src/sage/rings/polynomial/polynomial_element.pyx
+++ b/src/sage/rings/polynomial/polynomial_element.pyx
@@ -181,8 +181,12 @@ cdef class Polynomial(CommutativeAlgebraElement):

     EXAMPLE::

-        sage: R.<y> = QQ['y']
-        sage: S.<x> = R['x']
+        sage: R = QQ['x']['y']
+        sage: R
+        Univariate Polynomial Ring in y over Univariate Polynomial Ring in x
+        over Rational Field
+        sage: y = R.gen()
+        sage: x = R.base_ring().gen()
         sage: f = x*y; f
         y*x
         sage: type(f)
--- a/src/sage/schemes/generic/algebraic_scheme.py
+++ b/src/sage/schemes/generic/algebraic_scheme.py
@@ -194,7 +194,7 @@ def is_AlgebraicScheme(x):

     We create a more complicated closed subscheme::

-        sage: A, x = AffineSpace(10, QQ).objgens()
+        sage: A = AffineSpace(10, QQ)
         sage: X = A.subscheme([sum(x)]); X
         Closed subscheme of Affine Space of dimension 10 over Rational Field defined by:
         x0 + x1 + x2 + x3 + x4 + x5 + x6 + x7 + x8 + x9
 The third argument specifies the printing names of the generators
-of the homogenous coordinate ring. Using objgens() you can obtain
-both the space and the generators as ready to use variables.
+of the homogenous coordinate ring. Using the special syntax with
+``<`` and ``>>`` you can obtain both the space and the generators as ready to
+use variables.
mezzarobba commented 9 years ago

Changed author from Marc Mezzarobba to Marc Mezzarobba, Vincent Delecroix

mezzarobba commented 9 years ago

Reviewer: Vincent Delecroix, Marc Mezzarobba

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

485ae68Trac 5332: fix doctests and documentation
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from be20eb8 to 485ae68

videlec commented 9 years ago
comment:29

Replying to @mezzarobba:

  • I'm wondering why you made this change:

Right, it is not really better. Reverted.

  • Does this work? How?

It does not.

  • Shouldn't >> be > here? Also, I would add a mention of .<> without removing that of objgens().

done.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 485ae68 to 3a1e77e

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

3a1e77e#5332 fix doctest fix
mezzarobba commented 9 years ago
comment:31

I pushed a small fix to your fix (not tested, I'm having trouble with my development build at the moment).

One more comment: exemples like A, z = R.poly_ring().objgen() are typical cases where I would have kept the objgens version. But I have no problem with your version, do as you prefer.

If you are okay with my commits and assuming all tests pass, please feel free to set the ticket to positive review.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 3a1e77e to ea55f2a

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

ea55f2a#5332 fix errors in the last two commits
mezzarobba commented 9 years ago
comment:33

Turns out the tests didn't pass, but now they should.

videlec commented 9 years ago
comment:34

They do!

vbraun commented 9 years ago

Changed branch from public/5332 to ea55f2a