-
-
Notifications
You must be signed in to change notification settings - Fork 453
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
Fix some corner and special cases concerning localization of integral domains #33463
Comments
Commit: |
Author: Sebastian Oehms |
New commits:
|
comment:3
Can you also add this to the docstring (copied from the polynomial factorization):
and a Addendum: You can also leave off the |
comment:4
Please capitalise and use double colon here: - check that :trac:`33463` is fixed:
+ Check that :trac:`33463` is fixed:: |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:6
Replying to @tscrim:
Yes, you are right, Travis! But, I think instead of ignoring it it could by useful to pass the keyword to the |
comment:7
Replying to @slel:
Thanks for this hint, as well, Samuel! |
comment:8
That is fine with me, but - - ``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`` |
Branch pushed to git repo; I updated commit sha1. New commits:
|
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:11
Replying to @tscrim:
Sorry! |
comment:12
Thanks (although I would normally put the optional in parentheses). I am okay with the current branch. Samuel? |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:14
Replying to @tscrim:
Of course! Sorry again (that was not a good day). |
Reviewer: Travis Scrimshaw, Samuel Lelièvre |
comment:15
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
I wish one could change the display of the variables in the localisation. Something like:
where the generators would display as 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 shorter name 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
I have opened #33482 |
Changed branch from u/soehms/fix_corner_cases_localization_33463 to |
The aime of this ticket is to fix the following issues:
In addition a method
factor
is added.CC: @tscrim
Component: commutative algebra
Keywords: integral domain localization
Author: Sebastian Oehms
Branch/Commit:
fc1865a
Reviewer: Travis Scrimshaw, Samuel Lelièvre
Issue created by migration from https://trac.sagemath.org/ticket/33463
The text was updated successfully, but these errors were encountered: