Opened 5 years ago

Closed 5 years ago

#16238 closed defect (fixed)

Correct call convention for isogenies

Reported by: sbesnier Owned by:
Priority: major Milestone: sage-6.4
Component: elliptic curves Keywords: call isogeny
Cc: defeo, cremona Merged in:
Authors: Sébastien Besnier Reviewers: Peter Bruin
Report Upstream: N/A Work issues:
Branch: 028803c (Commits) Commit: 028803c8d16afbf85f149e61c08ebd6999f3c088
Dependencies: #12880, #16245 Stopgaps:

Description

The method __call__ of the class EllipticCurveIsogeny takes a second argument output_base_ring. It seems weird to me, do you find this is conventional?

Moreover, this method should be _call_ in order to make the functionalities of Map available.

Change History (14)

comment:1 Changed 5 years ago by sbesnier

  • Dependencies set to #12880

comment:2 follow-up: Changed 5 years ago by pbruin

It is not uncommon for __call__() to take extra (positional or keyword) arguments; this is used for example to implement E(x,y) as an alternative notation for E((x, y)) to construct a point on an elliptic curve.

I think that you can rename __call__() to _call_() if you also implement _call_with_args(), for example as follows (the non-standard calling convention seems to be a consequence of Cython restrictions):

  • src/sage/schemes/elliptic_curves/ell_curve_isogeny.py

    diff --git a/src/sage/schemes/elliptic_curves/ell_curve_isogeny.py b/src/sage/schemes/elliptic_curves/ell_curve_isogeny.py
    index 8b18590..7aa1583 100644
    a b class EllipticCurveIsogeny(Morphism): 
    10291029
    10301030        return outE2.point([R(outP[0]), R(outP[1]), R(1)], check=False)
    10311031
     1032    def _call_with_args(self, x, args=(), kwds={}):
     1033        return self._call_(x, *args, **kwds)
    10321034
    10331035    def __getitem__(self, i):
    10341036        self.__initialize_rational_maps()

However, I'm wondering if this output_base_ring argument is used for any purpose at all. You could try simply removing it; then _call_with_args() is not necessary either.

comment:3 in reply to: ↑ 2 Changed 5 years ago by defeo

However, I'm wondering if this output_base_ring argument is used for any purpose at all. You could try simply removing it; then _call_with_args() is not necessary either.

Same opinion here. Looks like bad design to me: we already have coercion and .change_ring to achieve the same result. These are standard idioms in Sage, while output_base_ring is not.

Except for one single doctest not shown in the ref manual, the third argument is not even documented.

Since you are at it, you could clean the code of __call__. It does a lot of manual checking of parents, which should be taken care of automatically by the coercion system if you switch to _call_.

comment:4 follow-up: Changed 5 years ago by sbesnier

  • Branch set to u/sbesnier/ticket/16238
  • Commit set to fbe7647092fba74971fe5e1e9c0cbe82e19d02ae

I've done the suggested modifications. The coercion staff is now handle by Map (the doctests concercing isogeny equality still fail, it'll be fix as soon as #11474 will be fix).

Currently, we can call phi((1,0)), but no phi(1,0) (in the same way we can call E(1,0)). Do you think it would be usefull to implement that? We should use something like:

def _call_with_args(self, x, y=None,kwd={}):
    return self._call_(self.__E2(x,y))

But this simple code doesn't work at all! Indeed, when we call for example phi(0,0), the first arg is first coerced to phi.__E1(0) and then passed to _call_with_args.

Should I overload __call__, in this way:

def __call__(self,x,y=None):
    if y is None:
       return Map.__call__(self,x)
    else:
       return Map.__call__(self,(x,y))

?

But this looks a bit creepy to me.


New commits:

b486f24Fix parents, domain, codomain and _composition_ isogenies.
73d99afCorrects the precedent commit.
8c5cdcdFix composition.
fbe7647Clean _call_, __call__ becomes _call_, second argment is removed.

comment:5 Changed 5 years ago by sbesnier

  • Status changed from new to needs_review

comment:6 in reply to: ↑ 4 ; follow-up: Changed 5 years ago by defeo

Currently, we can call phi((1,0)), but no phi(1,0) (in the same way we can call E(1,0)). Do you think it would be usefull to implement that?

I don't like it. Even phi((1,0)) is distasteful in my opinion.

comment:7 in reply to: ↑ 6 Changed 5 years ago by pbruin

Replying to defeo:

Currently, we can call phi((1,0)), but no phi(1,0) (in the same way we can call E(1,0)). Do you think it would be usefull to implement that?

I don't like it. Even phi((1,0)) is distasteful in my opinion.

The notation phi((1, 0)) presumably works automatically because (1, 0) is an object that can be coerced into E. It may not be the most expressive notation, but I see no reason not to keep it. I think extending this to allow phi(1, 0) is unnecessary.

comment:8 Changed 5 years ago by git

  • Commit changed from fbe7647092fba74971fe5e1e9c0cbe82e19d02ae to bae05ef7ae29f12d1b8258b09fb9a95ba9d62711

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

09fdbedCorrects stylistic staff in doctests.
e1e8c29Merge branch 'ticket/12880' into ticket/16245
de93bbaStylistic modification for doctests.
695ed4eMerge branch 'ticket/16245' into ticket/16238
bae05efCorrects stylistic staff in doctests.

comment:9 Changed 5 years ago by sbesnier

  • Dependencies changed from #12880 to #12880, #16245

comment:10 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:11 Changed 5 years ago by cremona

  • Cc cremona added

comment:12 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:13 Changed 5 years ago by pbruin

  • Branch changed from u/sbesnier/ticket/16238 to u/pbruin/16238-isogeny_call
  • Commit changed from bae05ef7ae29f12d1b8258b09fb9a95ba9d62711 to 028803c8d16afbf85f149e61c08ebd6999f3c088
  • Reviewers set to Peter Bruin
  • Status changed from needs_review to positive_review

Merged #16245 and made a trivial reviewer patch; positive review conditional on #16245.

comment:14 Changed 5 years ago by vbraun

  • Branch changed from u/pbruin/16238-isogeny_call to 028803c8d16afbf85f149e61c08ebd6999f3c088
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.