Opened 7 months ago

Closed 7 months ago

#33463 closed defect (fixed)

Fix some corner and special cases concerning localization of integral domains

Reported by: Sebastian Oehms Owned by:
Priority: major Milestone: sage-9.6
Component: commutative algebra Keywords: integral domain localization
Cc: Travis Scrimshaw Merged in:
Authors: Sebastian Oehms Reviewers: Travis Scrimshaw, Samuel Lelièvre
Report Upstream: N/A Work issues:
Branch: fc1865a (Commits, GitHub, GitLab) Commit: fc1865ab76a21ce76a621ee0b6a678a815138a02
Dependencies: Stopgaps:

Status badges

Description

The aime of this ticket is to fix the following issues:

sage: L = ZZ.localization(5)
sage: o = L(0)
sage: o.is_unit()
Traceback (most recent call last)
...
RecursionError: maximum recursion depth exceeded while calling a Python object
sage: R = ZZ.localization(5)
sage: S = R.localization(~5)
Traceback (most recent call last)
...
TypeError: no conversion of this rational to integer
sage: Lau.<u, v> = LaurentPolynomialRing(ZZ)
sage: LauL = Lau.localization(u+1)
sage: LauL(~u)
Traceback (most recent call last)
...
NotImplementedError:

In addition a method factor is added.

Change History (18)

comment:1 Changed 7 months ago by Sebastian Oehms

Branch: u/soehms/fix_corner_cases_localization_33463

comment:2 Changed 7 months ago by Sebastian Oehms

Authors: Sebastian Oehms
Cc: Travis Scrimshaw added
Commit: ff2e83e57fd33a6d11de74f41141f584d37e08ef
Status: newneeds_review

New commits:

ff2e83e33463: initial version

comment:3 Changed 7 months ago by Travis Scrimshaw

Can you also add this to the docstring (copied from the polynomial factorization):

   Return the factorization of this polynomial.

   INPUT:

   - ``proof`` -- ignored

and a proof=None default argument (for compatibility, although not every factor() implementation seems to support it)?

Addendum: You can also leave off the INPUT: too if you want since it is ignored.

Last edited 7 months ago by Travis Scrimshaw (previous) (diff)

comment:4 Changed 7 months ago by Samuel Lelièvre

Please capitalise and use double colon here:

-        check that :trac:`33463` is fixed:
+        Check that :trac:`33463` is fixed::

comment:5 Changed 7 months ago by git

Commit: ff2e83e57fd33a6d11de74f41141f584d37e08ef5df7f557218f93f06506453162ca0463b8c0609f

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

5df7f5533464: add kw proof to factor and style fixes

comment:6 in reply to:  3 Changed 7 months ago by Sebastian Oehms

Replying to tscrim:

Can you also add this to the docstring (copied from the polynomial factorization):

   Return the factorization of this polynomial.

   INPUT:

   - ``proof`` -- ignored

and a proof=None default argument (for compatibility, although not every factor() implementation seems to support it)?

Addendum: You can also leave off the INPUT: too if you want since it is ignored.

Yes, you are right, Travis! But, I think instead of ignoring it it could by useful to pass the keyword to the factor method over the base ring in case it is given explicitely.

comment:7 in reply to:  4 Changed 7 months ago by Sebastian Oehms

Replying to slelievre:

Please capitalise and use double colon here:

-        check that :trac:`33463` is fixed:
+        Check that :trac:`33463` is fixed::

Thanks for this hint, as well, Samuel!

comment:8 Changed 7 months ago by Travis Scrimshaw

That is fine with me, but factor() still needs a one-line description. Also, one other tweak:

-        - ``proof`` -- optional (default ``None``). If given it is passed
-          to the corresponding method of the numerator of ``self``.
+        - ``proof`` -- (optional) if given it is passed to the
+          corresponding method of the numerator of ``self``
Last edited 7 months ago by Travis Scrimshaw (previous) (diff)

comment:9 Changed 7 months ago by git

Commit: 5df7f557218f93f06506453162ca0463b8c0609fceeeebc796ba03e2f1617bde932449584d399cf2

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

ceeeebc33463: add missing description of meth factor

comment:10 Changed 7 months ago by git

Commit: ceeeebc796ba03e2f1617bde932449584d399cf2d48dc281ef58374105106c381c56033966f7f747

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

d48dc2833463: once again

comment:11 in reply to:  8 Changed 7 months ago by Sebastian Oehms

Replying to tscrim:

That is fine with me, but factor() still needs a one-line description. Also, one other tweak:

Sorry!

comment:12 Changed 7 months ago by Travis Scrimshaw

Thanks (although I would normally put the optional in parentheses). I am okay with the current branch. Samuel?

comment:13 Changed 7 months ago by git

Commit: d48dc281ef58374105106c381c56033966f7f747fc1865ab76a21ce76a621ee0b6a678a815138a02

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

fc1865a33463: another one

comment:14 in reply to:  12 Changed 7 months ago by Sebastian Oehms

Replying to tscrim:

although I would normally put the optional in parentheses

Of course! Sorry again (that was not a good day).

comment:15 Changed 7 months ago by Travis Scrimshaw

Reviewers: Travis Scrimshaw, Samuel Lelièvre
Status: needs_reviewpositive_review

No problem. Thank you for the fixes.

I am going to set this to a positive review since the patchbot is (morally) green. Samuel, if you have other changes you want, feel free to revert.

comment:16 Changed 7 months ago by Samuel Lelièvre

I wish one could change the display of the variables in the localisation.

Something like:

sage: P.<X, Y> = QQ[]
sage: L.<x, y> = P.localization(X - Y)

where the generators would display as X, Y in P and as x, y in L.

That could be another ticket.

One could remove a pair of parentheses here:

-        fac = [(P(f), e) for (f, e) in F]
+        fac = [(P(f), e) for f, e in F]

but leaving them is fine too and even emphasizes the transformation.

The shorter name extra_units might shorten some lines with respect to additional_units.

One could split this line:

-            additional_units = [au for au in additional_units if ~au not in base_ring._additional_units]  # :trac:`33463`
+            additional_units = [u for u in additional_units
+                                if ~u not in base_ring._additional_units]

These are all minor points. No need to revert positive review.

comment:17 Changed 7 months ago by Sebastian Oehms

That could be another ticket.

I have opened #33482

comment:18 Changed 7 months ago by Volker Braun

Branch: u/soehms/fix_corner_cases_localization_33463fc1865ab76a21ce76a621ee0b6a678a815138a02
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.