Opened 3 years ago

Closed 3 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) Commit: e0ec6ecca8f6637b3978e25d54b2345feefd05e2
Dependencies: #12600 Stopgaps:

Description (last modified by mmezzarobba)

We deprecate the usage of zero_element/one_element in favor of zero/one.

To avoid duplication we also upgrade slightly the category of polyhedra.

see also: #5891

follow up: #17692

Change History (26)

comment:1 Changed 3 years ago by vdelecroix

  • Authors set to Vincent Delecroix
  • Branch set to u/vdelecroix/17694
  • Cc ncohen slelievre added
  • Commit set to c0a0bbbe64c8524c98c88f342c62cc426c604260
  • Description modified (diff)
  • Status changed from new to needs_review

New commits:

d3e1ad5trac #17694: upgrade the category of polyhedra
c0a0bbbtrac #17694: one_element->one and zero_element->zero

comment:2 Changed 3 years ago by vdelecroix

  • Description modified (diff)

comment:3 Changed 3 years ago by mmezzarobba

  • Description modified (diff)

comment:4 Changed 3 years ago by ncohen

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 3 years ago by jdemeyer

Replying to vdelecroix:

To avoid duplication we also upgrade slightly the category of polyhedra.

Please explain...

comment:6 Changed 3 years ago by vdelecroix

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: Changed 3 years ago by jdemeyer

The polyhedra change worries me (as potential reviewer) because I don't fully understand how categories work.

comment:8 in reply to: ↑ 7 Changed 3 years ago by vdelecroix

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 3 years ago by git

  • Commit changed from c0a0bbbe64c8524c98c88f342c62cc426c604260 to cccd10cd17efc8d597f55857fbf8b0a052447ddd

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

e3aa703trac #17692: rewrite __invert__ (and delete some)
a1a9db3trac #17694: remove the zero test
cccd10ctrac #17694: place deprecated_function_alias where appropriate

comment:10 Changed 3 years ago by git

  • Commit changed from cccd10cd17efc8d597f55857fbf8b0a052447ddd to 9ac0f9aeee0783d92dbf2bd652d1b6209f7914bc

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

9ac0f9atrac #17694: place deprecated_function_alias where appropriate

comment:11 Changed 3 years ago by vdelecroix

Sorry... wrong commit. I messed up with #17692. Fixed now.

comment:12 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work
deprecation(17694, ".one() is deprecated. Please use .one() instead.")

comment:13 Changed 3 years ago by git

  • Commit changed from 9ac0f9aeee0783d92dbf2bd652d1b6209f7914bc to 0ba31f7e48bc8f7db62c9f0188fa2c0bc8ed4957

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

0ba31f7trac #17694: fix deprecation message

comment:14 Changed 3 years ago by vdelecroix

  • Status changed from needs_work to needs_review

comment:15 follow-up: Changed 3 years ago by mmezzarobba

Lgtm, but I'd prefer waiting for a second opinion as Nathann and Jeroen appeared to have concerns.

comment:16 Changed 3 years ago by jdemeyer

I don't have further concerns.

comment:17 in reply to: ↑ 15 Changed 3 years ago by ncohen

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 3 years ago by mmezzarobba

  • Reviewers set to Nathann Cohen, Jeroen Demeyer, Marc Mezzarobba
  • Status changed from needs_review to positive_review

comment:19 follow-up: Changed 3 years ago by mmezzarobba

  • 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: Changed 3 years ago by 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:21 in reply to: ↑ 20 Changed 3 years ago by mmezzarobba

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?

#12600

comment:22 Changed 3 years ago by git

  • Commit changed from 0ba31f7e48bc8f7db62c9f0188fa2c0bc8ed4957 to e0ec6ecca8f6637b3978e25d54b2345feefd05e2

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

5e83b95Add epsilon() method to rings
f1a086aFix doctest
e0ec6ectrac #17694: merge #12600

comment:23 Changed 3 years ago by vdelecroix

  • Status changed from needs_work to needs_review

comment:24 Changed 3 years ago by vdelecroix

  • Dependencies set to #12600

comment:25 Changed 3 years ago by mmezzarobba

  • Status changed from needs_review to positive_review

comment:26 Changed 3 years ago by vbraun

  • Branch changed from u/vdelecroix/17694 to e0ec6ecca8f6637b3978e25d54b2345feefd05e2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.