Opened 7 years ago
Closed 7 years ago
#17694 closed enhancement (fixed)
zero vs zero_element / one vs one_element
Reported by: | vdelecroix | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-6.5 |
Component: | algebra | Keywords: | |
Cc: | ncohen, slelievre | Merged in: | |
Authors: | Vincent Delecroix | Reviewers: | Nathann Cohen, Jeroen Demeyer, Marc Mezzarobba |
Report Upstream: | N/A | Work issues: | |
Branch: | e0ec6ec (Commits, GitHub, GitLab) | Commit: | e0ec6ecca8f6637b3978e25d54b2345feefd05e2 |
Dependencies: | #12600 | Stopgaps: |
Description (last modified by )
Change History (26)
comment:1 Changed 7 years ago by
- Branch set to u/vdelecroix/17694
- Cc ncohen slelievre added
- Commit set to c0a0bbbe64c8524c98c88f342c62cc426c604260
- Description modified (diff)
- Status changed from new to needs_review
comment:2 Changed 7 years ago by
- Description modified (diff)
comment:3 Changed 7 years ago by
- Description modified (diff)
comment:4 Changed 7 years ago by
Hello,
There are some zero_element
that you rename to zero
without adding a deprecated copy. I wondered if perhaps the deprecated copy was inherited, but found something weird:
sage: L = LazyPowerSeriesRing(QQ) sage: L.zero # zero_element appears when you press tab L.zero L.zero_element L.zero_ideal sage: L.zero_element() ... AttributeError: 'LazyPowerSeriesRing' object has no attribute 'zero_element'
So it seems that this function, for instance, has no deprecated zero_element
. And also that the function still appears in the tab expansion, which I cannot explain.
Nathann
comment:5 in reply to: ↑ description Changed 7 years ago by
Replying to vdelecroix:
To avoid duplication we also upgrade slightly the category of polyhedra.
Please explain...
comment:6 Changed 7 years ago by
Replying to ncohen:
Right. I guess that the problem is that Rings
is not a sub category of additive monoid and multiplicative monoid! I really do not want to fix it within categories as there is an interplay with Nicolas axioms.
I will add a commit that takes care of LazyPowerSeries
and have a look at other rings which do not inherit from sage.rings.ring.Ring
.
Replying to jdemeyer:
Replying to vdelecroix:
To avoid duplication we also upgrade slightly the category of polyhedra.
Please explain...
The polyhedra do have a sum (Minkowski sum) and a product (cartesian product). The method zero_element
was implemented but the category was AdditiveMagmas. So I switched to AdditiveMonoids (i.e. Magma + unit) and only rename the method zero_element()
to zero()
. And the ParentMethods of the category takes care of the deprecation.
comment:7 follow-up: ↓ 8 Changed 7 years ago by
The polyhedra change worries me (as potential reviewer) because I don't fully understand how categories work.
comment:8 in reply to: ↑ 7 Changed 7 years ago by
Replying to jdemeyer:
The polyhedra change worries me (as potential reviewer) because I don't fully understand how categories work.
For the purpose of this ticket, you only need to know that ParentMethods
and ElementMethods
in the categories are respectively inherited to the concrete Parent
and Element
(via a dirty mechanism of dynamic classes created on the fly).
An other option is to revert the changes and add a deprecated_function_alias
inside the polyhedron class.
Vincent
comment:9 Changed 7 years ago by
- Commit changed from c0a0bbbe64c8524c98c88f342c62cc426c604260 to cccd10cd17efc8d597f55857fbf8b0a052447ddd
comment:10 Changed 7 years ago by
- Commit changed from cccd10cd17efc8d597f55857fbf8b0a052447ddd to 9ac0f9aeee0783d92dbf2bd652d1b6209f7914bc
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
9ac0f9a | trac #17694: place deprecated_function_alias where appropriate
|
comment:11 Changed 7 years ago by
Sorry... wrong commit. I messed up with #17692. Fixed now.
comment:12 Changed 7 years ago by
- Status changed from needs_review to needs_work
deprecation(17694, ".one() is deprecated. Please use .one() instead.")
comment:13 Changed 7 years ago by
- Commit changed from 9ac0f9aeee0783d92dbf2bd652d1b6209f7914bc to 0ba31f7e48bc8f7db62c9f0188fa2c0bc8ed4957
Branch pushed to git repo; I updated commit sha1. New commits:
0ba31f7 | trac #17694: fix deprecation message
|
comment:14 Changed 7 years ago by
- Status changed from needs_work to needs_review
comment:15 follow-up: ↓ 17 Changed 7 years ago by
Lgtm, but I'd prefer waiting for a second opinion as Nathann and Jeroen appeared to have concerns.
comment:16 Changed 7 years ago by
I don't have further concerns.
comment:17 in reply to: ↑ 15 Changed 7 years ago by
Hello,
Lgtm, but I'd prefer waiting for a second opinion as Nathann and Jeroen appeared to have concerns.
Well, all I wanted to do with this branch is make sure that all functions that have been removed are now deprecated. If this is done I have no objection at all.
Nathann
comment:18 Changed 7 years ago by
- Reviewers set to Nathann Cohen, Jeroen Demeyer, Marc Mezzarobba
- Status changed from needs_review to positive_review
comment:19 follow-up: ↓ 20 Changed 7 years ago by
- Status changed from positive_review to needs_work
Turns out I didn't notice a merge conflict with f1a086ae
(which also reformats the example in a way that will hopefully make such conflicts less frequent).
comment:20 in reply to: ↑ 19 ; follow-up: ↓ 21 Changed 7 years ago by
Replying to mmezzarobba:
Turns out I didn't notice a merge conflict with
f1a086ae
(which also reformats the example in a way that will hopefully make such conflicts less frequent).
Which ticket?
comment:21 in reply to: ↑ 20 Changed 7 years ago by
Replying to vdelecroix:
Replying to mmezzarobba:
Turns out I didn't notice a merge conflict with
f1a086ae
(which also reformats the example in a way that will hopefully make such conflicts less frequent).Which ticket?
comment:22 Changed 7 years ago by
- Commit changed from 0ba31f7e48bc8f7db62c9f0188fa2c0bc8ed4957 to e0ec6ecca8f6637b3978e25d54b2345feefd05e2
comment:23 Changed 7 years ago by
- Status changed from needs_work to needs_review
comment:24 Changed 7 years ago by
- Dependencies set to #12600
comment:25 Changed 7 years ago by
- Status changed from needs_review to positive_review
comment:26 Changed 7 years ago by
- Branch changed from u/vdelecroix/17694 to e0ec6ecca8f6637b3978e25d54b2345feefd05e2
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
trac #17694: upgrade the category of polyhedra
trac #17694: one_element->one and zero_element->zero