Opened 5 years ago

Closed 5 years ago

Last modified 2 years ago

#15377 closed defect (fixed)

improve documentation for projective normalize_coordinates

Reported by: bhutz Owned by: bhutz
Priority: trivial Milestone: sage-5.13
Component: algebraic geometry Keywords: sage-days55, days54
Cc: Merged in: sage-5.13.beta3
Authors: Ben Hutz Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

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.

Attachments (1)

trac_15377_improve_documentation_normalize_coordinates.patch (3.2 KB) - added by bhutz 5 years ago.
comma removed

Download all attachments as: .zip

Change History (11)

comment:1 Changed 5 years ago by bhutz

  • Status changed from new to needs_review

added doumentation and examples. Cleaned up formatting of examples.

comment:2 Changed 5 years ago by tscrim

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

Last edited 5 years ago by tscrim (previous) (diff)

comment:3 Changed 5 years ago by bhutz

  • Owner changed from (none) to bhutz

comment:4 Changed 5 years ago by atowsley

  • Status changed from needs_review to positive_review

It fixed the typo and clarified the functionality.

Examples do what they should.

doctest looks good.

-Adam

comment:5 Changed 5 years ago by tscrim

  • Status changed from positive_review to needs_work

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?

comment:6 Changed 5 years ago by bhutz

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

comment:7 Changed 5 years ago by tscrim

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!

Changed 5 years ago by bhutz

comma removed

comment:8 Changed 5 years ago by tscrim

  • Authors set to Benjamin Hutz
  • Keywords days54 added
  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_work to positive_review

Positive review. Thanks.

comment:9 Changed 5 years ago by jdemeyer

  • Merged in set to sage-5.13.beta3
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:10 Changed 2 years ago by chapoton

  • Authors changed from Benjamin Hutz to Ben Hutz
Note: See TracTickets for help on using tickets.