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

improve documentation for projective normalize_coordinates #15377

Closed
bhutz opened this issue Nov 7, 2013 · 13 comments
Closed

improve documentation for projective normalize_coordinates #15377

bhutz opened this issue Nov 7, 2013 · 13 comments

Comments

@bhutz
Copy link

bhutz commented Nov 7, 2013

For some base rings (such as polynomial rings over a field), the output of the function may seem counter intuitive even though the gcd is correct. The documentation is inadequate to describe this situation.

Component: algebraic geometry

Keywords: sage-days55, days54

Author: Ben Hutz

Reviewer: Travis Scrimshaw

Merged: sage-5.13.beta3

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

@bhutz bhutz added this to the sage-5.13 milestone Nov 7, 2013
@bhutz
Copy link
Author

bhutz commented Nov 7, 2013

comment:1

added doumentation and examples. Cleaned up formatting of examples.

@tscrim
Copy link
Collaborator

tscrim commented Nov 8, 2013

comment:2

One thing for line 394; instead of "Beware" (or "Be aware"), I think this would be best formatted as

.. WARNING::

    The gcd depends on the base ring.

After this it will be a positive review.

Edit - one other thing, could you remove the comma , from line 449.

Best,

Travis

@bhutz bhutz self-assigned this Nov 8, 2013
@sagetrac-atowsley
Copy link
Mannequin

sagetrac-atowsley mannequin commented Nov 8, 2013

comment:4

It fixed the typo and clarified the functionality.

Examples do what they should.

doctest looks good.

-Adam

@tscrim
Copy link
Collaborator

tscrim commented Nov 8, 2013

comment:5

I missed this the first go-around but you need to have a proper commit message.

Also why did you choose not to use the .. WARNING:: for the gcd dependency on the base ring?

@bhutz
Copy link
Author

bhutz commented Nov 8, 2013

comment:6

opps. I forgot the commit message. The warning field is a good suggestion and has now been implemented for this patch.

@tscrim
Copy link
Collaborator

tscrim commented Nov 8, 2013

comment:7

Last thing; I noticed (I edited my first comment) is that the comma here

A polynomial ring over a ring, gives the more intuitive result.

should be removed. Thanks!

@bhutz
Copy link
Author

bhutz commented Nov 8, 2013

Attachment: trac_15377_improve_documentation_normalize_coordinates.patch.gz

comma removed

@tscrim
Copy link
Collaborator

tscrim commented Nov 8, 2013

Changed keywords from sage-days55 to sage-days55, days54

@tscrim
Copy link
Collaborator

tscrim commented Nov 8, 2013

Author: Benjamin Hutz

@tscrim
Copy link
Collaborator

tscrim commented Nov 8, 2013

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Nov 8, 2013

comment:8

Positive review. Thanks.

@jdemeyer
Copy link

Merged: sage-5.13.beta3

@fchapoton
Copy link
Contributor

Changed author from Benjamin Hutz to Ben Hutz

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

4 participants