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:  sage9.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: 
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
Branch:  → u/soehms/fix_corner_cases_localization_33463 

comment:2 Changed 7 months ago by
Authors:  → Sebastian Oehms 

Cc:  Travis Scrimshaw added 
Commit:  → ff2e83e57fd33a6d11de74f41141f584d37e08ef 
Status:  new → needs_review 
comment:3 followup: 6 Changed 7 months ago by
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.
comment:4 followup: 7 Changed 7 months ago by
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
Commit:  ff2e83e57fd33a6d11de74f41141f584d37e08ef → 5df7f557218f93f06506453162ca0463b8c0609f 

Branch pushed to git repo; I updated commit sha1. New commits:
5df7f55  33464: add kw proof to factor and style fixes

comment:6 Changed 7 months ago by
Replying to tscrim:
Can you also add this to the docstring (copied from the polynomial factorization):
Return the factorization of this polynomial. INPUT:  ``proof``  ignoredand a
proof=None
default argument (for compatibility, although not everyfactor()
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 Changed 7 months ago by
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 followup: 11 Changed 7 months ago by
That is fine with me, but factor()
still needs a oneline 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``
comment:9 Changed 7 months ago by
Commit:  5df7f557218f93f06506453162ca0463b8c0609f → ceeeebc796ba03e2f1617bde932449584d399cf2 

Branch pushed to git repo; I updated commit sha1. New commits:
ceeeebc  33463: add missing description of meth factor

comment:10 Changed 7 months ago by
Commit:  ceeeebc796ba03e2f1617bde932449584d399cf2 → d48dc281ef58374105106c381c56033966f7f747 

Branch pushed to git repo; I updated commit sha1. New commits:
d48dc28  33463: once again

comment:11 Changed 7 months ago by
Replying to tscrim:
That is fine with me, but
factor()
still needs a oneline description. Also, one other tweak:
Sorry!
comment:12 followup: 14 Changed 7 months ago by
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
Commit:  d48dc281ef58374105106c381c56033966f7f747 → fc1865ab76a21ce76a621ee0b6a678a815138a02 

Branch pushed to git repo; I updated commit sha1. New commits:
fc1865a  33463: another one

comment:14 Changed 7 months ago by
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
Reviewers:  → Travis Scrimshaw, Samuel Lelièvre 

Status:  needs_review → positive_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
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:18 Changed 7 months ago by
Branch:  u/soehms/fix_corner_cases_localization_33463 → fc1865ab76a21ce76a621ee0b6a678a815138a02 

Resolution:  → fixed 
Status:  positive_review → closed 
New commits:
33463: initial version