Skip to content
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

Add __mod__ to coercion model #269

Closed
sagetrac-dmharvey mannequin opened this issue Feb 17, 2007 · 41 comments
Closed

Add __mod__ to coercion model #269

sagetrac-dmharvey mannequin opened this issue Feb 17, 2007 · 41 comments

Comments

@sagetrac-dmharvey
Copy link
Mannequin

sagetrac-dmharvey mannequin commented Feb 17, 2007

Add __mod__ to the coercion model for Element and change __mod__ to _mod_ where applicable.

We change the behaviour of __mod__ for integer mod rings: the following (which is mathematically meaningless) is now an error:

             sage: a = next_prime(2**31)
             sage: b = Integers(a)(100)
             sage: a % b
             Traceback (most recent call last):
             ...
             ZeroDivisionError: reduction modulo 100 not defined

Apart from this, this branch does not change any semantics of remaindering.

As proof-of-concept for refactoring more generally the arithmetic methods on Element, the implementation of __mod__ is different from the usual implementation:

  1. _mod_ will be added to Element and not RingElement or similar.
  2. The prototype is cpdef _mod_(self, other) with no typing for the arguments or return value.
  3. We do not implement the in-place __imod__ which is useless anyway with coercion.

My intention is that eventually all arithmetic methods should be implemented like __mod__.

CC: @videlec

Component: coercion

Author: Jeroen Demeyer

Branch/Commit: aca3398

Reviewer: Vincent Delecroix

Issue created by migration from https://trac.sagemath.org/ticket/269

@sagetrac-mabshoff sagetrac-mabshoff mannequin added this to the sage-2.9.1 milestone Sep 6, 2007
@sagetrac-cwitty
Copy link
Mannequin

sagetrac-cwitty mannequin commented Oct 6, 2007

Replying to @sagetrac-dmharvey:

So far we only have add, sub, neg, various versions of mul, and div.

We also need floordiv, mod, invert, pow.

These would be very useful in p-adics, and need to happen for other reasons too.

Is exact division (that is, division where the caller knows that there is no remainder) common enough to belong here? (There's an implementation for Integer already, where the method is called divide_knowing_divisible_by() and calls mpz_divexact().)

@sagetrac-jbmohler
Copy link
Mannequin

sagetrac-jbmohler mannequin commented Mar 19, 2008

comment:4

Replying to @sagetrac-cwitty:

Replying to @sagetrac-dmharvey:

So far we only have add, sub, neg, various versions of mul, and div.

We also need floordiv, mod, invert, pow.

These would be very useful in p-adics, and need to happen for other reasons too.

Is exact division (that is, division where the caller knows that there is no remainder) common enough to belong here? (There's an implementation for Integer already, where the method is called divide_knowing_divisible_by() and calls mpz_divexact().)

I definitely think that exact division should be included in the arithmetic architecture. This is not because it's common, but because there is a bunch of daft coercion code in most !floordiv! implementations. All this needs to be fixed and the fix is a common boiler-plate prefix on these functions. Or, a much better alternative is to include it in the arithmetic hierarchy and write that boilerplate coercion once at the head of the tree.

I think that gcd (and maybe xgcd) should also be on this list.

@sagetrac-jbmohler
Copy link
Mannequin

sagetrac-jbmohler mannequin commented Mar 19, 2008

comment:5

Oh, and quo_rem should be here too -- #383

@jdemeyer jdemeyer modified the milestones: sage-5.11, sage-5.12 Aug 13, 2013
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.1, sage-6.2 Jan 30, 2014
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.2, sage-6.3 May 6, 2014
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.3, sage-6.4 Aug 10, 2014
@jdemeyer

This comment has been minimized.

@jdemeyer jdemeyer changed the title add floordiv, mod, invert, pow to arithmetic architecture (at least in RingElement) Add floordiv and mod to arithmetic architecture (at least in RingElement) Sep 2, 2014
@jdemeyer

This comment has been minimized.

@jdemeyer jdemeyer changed the title Add floordiv and mod to arithmetic architecture (at least in RingElement) Add __mod__ to coercion model Jan 19, 2016
@jdemeyer jdemeyer modified the milestones: sage-6.4, sage-7.1 Jan 19, 2016
@jdemeyer
Copy link

Dependencies: #2034

@jdemeyer
Copy link

Author: Jeroen Demeyer

@jdemeyer

This comment has been minimized.

@jdemeyer

This comment has been minimized.

@jdemeyer

This comment has been minimized.

@videlec
Copy link
Contributor

videlec commented May 31, 2016

comment:27

About comment:19 I guess that the rationale is that we should follow what was done until now. If you want to change the cpdef methods without declaring them as Element or ModuleElement or whatever I guess you should open a new ticket.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 5, 2016

Changed commit from 665b8d1 to b48a839

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 5, 2016

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

b48a839Implement `__mod__` in the coercion model

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 14, 2016

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

35b3dfaImplement `__mod__` in the coercion model

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 14, 2016

Changed commit from b48a839 to 35b3dfa

@jdemeyer

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 14, 2016

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

8079508Implement `__mod__` in the coercion model

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 14, 2016

Changed commit from 35b3dfa to 8079508

@videlec
Copy link
Contributor

videlec commented Jul 13, 2016

comment:32

Please, add doctests for __mod__ in sage.rings.finite_rings.integer_mod.

@videlec
Copy link
Contributor

videlec commented Jul 13, 2016

Reviewer: Vincent Delecroix

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 14, 2016

Changed commit from 8079508 to defac69

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 14, 2016

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

defac69Reimplement `__mod__` for integer mods

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 14, 2016

Changed commit from defac69 to 1d53754

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 14, 2016

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

1d53754Reimplement `__mod__` for integer mods

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 15, 2016

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

aca3398Fix doctests for changed exception type

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 15, 2016

Changed commit from 1d53754 to aca3398

@jdemeyer
Copy link

comment:38

Thanks!

@vbraun
Copy link
Member

vbraun commented Jul 27, 2016

Changed branch from u/jdemeyer/add___mod___to_coercion_model to aca3398

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants