Opened 7 years ago

Closed 6 years ago

#14212 closed enhancement (fixed)

add optional "names" argument to absolute_ideal

Reported by: mmanes Owned by: davidloeffler
Priority: minor Milestone: sage-5.12
Component: number fields Keywords: absolute ideal, absolute field,
Cc: Merged in: sage-5.12.beta3
Authors: David Lukas, Michelle Manes Reviewers: Alina Bucur
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by mmanes)

The absolute_ideal method uses 'a' as the name of the generator of the absolute_field, which may conflict with a user-chosen generator for the absolute_field (if that has already been done) or with some other variable that's in use.

This small fix adds an optional "names" argument so that a user can force agreement between the absolute_ideal and absolute_field if it was already constructed. If no name is passed, the default 'a' is used.

Most recent change to the patch adds a doctest for the optional names argument.

Attachments (1)

trac_14212.2.patch (2.7 KB) - added by mmanes 6 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 7 years ago by aly.deines

  • Status changed from new to needs_review

Several of the functions are missing either INPUT: OUTPUT: or both. Besides that it looks good.

comment:2 Changed 7 years ago by aly.deines

  • Status changed from needs_review to needs_work

comment:3 Changed 7 years ago by mmanes

  • Status changed from needs_work to needs_review

comment:4 Changed 7 years ago by aly.deines

  • Status changed from needs_review to needs_work

You need to add some examples that use the new functionality and get tested.

comment:5 Changed 7 years ago by mmanes

  • Description modified (diff)
  • Status changed from needs_work to needs_review

comment:6 Changed 6 years ago by kthompson

  • Status changed from needs_review to positive_review

However, my mind has been changed as this now does not work on ANYONE's machine.

Last edited 6 years ago by kthompson (previous) (diff)

comment:7 Changed 6 years ago by kthompson

  • Status changed from positive_review to needs_work

Changed 6 years ago by mmanes

comment:8 Changed 6 years ago by mmanes

trac_14212.2.patch is the correct patch. Please only apply that one.

comment:9 Changed 6 years ago by alina

  • Status changed from needs_work to positive_review

It installed, passed doctests.

I tested functionality on the examples provided and a few others of my own and it works as it's supposed to.

comment:10 Changed 6 years ago by jdemeyer

  • Status changed from positive_review to needs_info

Please add your real name as Reviewer.

comment:11 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:12 Changed 6 years ago by alina

  • Reviewers set to Alina Bucur
  • Status changed from needs_info to positive_review

comment:13 Changed 6 years ago by jdemeyer

  • Merged in set to sage-5.12.beta3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.