Need advice on PR attempting to modify coercion for Python int type

62 views
Skip to first unread message

Giacomo Pope

unread,
Feb 20, 2024, 8:04:20 AM2/20/24
to sage-devel
Hey all, 

I have been trying to work on a fix for scalar multiplication of points on elliptic curves over finite fields. The issue at the moment is that when we multiply by a Sage type, such as an Integer, the coercion system discovers the action via `_acted_upon_` and a fast method via Pari is called.

However, when the scalar multiplication is called with a Python `int`, then this falls through to `IntegerMulAction` which instead performs the scalar multiplication using Sage defined addition and doubling which is much slower (about 10x slower by the timing I have in the PR).

My initial fix was to simply add a `__mul__` method for elliptic curve points, but through conversations with the reviewers of the PR, instead a fix was proposed within the `discover_action()` method, which attempts a precomposition from python `int` to Sage `Integer` which then allows for the discovery of the action for `_acted_upon_`.

This fix "worked" in that the action works fine for my elliptic curve example and the speed for `int` and `Integer` now are both fast, but the CI tests run and it seems my change has totally broken Sage.

I really want to do the correct fix here, but I don't understand how much change could have broken so much internally as I thought my fix would only discover good actions... I would really appreciate some support in finding the right solution for this.


Thank you! 

Dima Pasechnik

unread,
Feb 20, 2024, 9:17:57 AM2/20/24
to sage-...@googlegroups.com
Did you try running tests locally?
Our CI sometimes acts up

Giacomo Pope

unread,
Feb 20, 2024, 9:53:26 AM2/20/24
to sage-devel
Yes, I tried locally and although the little example for elliptic curves works and shows a "fixed" issue with multiplication by an int, several new multiplications fail.

Picking one of these tests at random (the whole logos failure is 40Mb!!) I think I have identified one of the issues. 

Before I was accessing the Integer class from _Integer from the snippet from parent.pyx. However this seems to be None for certain files and so my coercion was breaking. By instead importing ZZ directly from the right file this seems tp have fixed some tests so I will commit this and check how the whole CI does. I'm not convinced what I did it "best" yet but I will be interested to see if my breaking change was simply a missing import for certain files.

Giacomo Pope

unread,
Feb 20, 2024, 5:23:15 PM2/20/24
to sage-devel
To follow up on this, with a few more tweaks I have all the CI passing. If a coercion expert wants to review the PR I would appreciate that, but I'm hopeful that this change is a net positive for sagemath.
Reply all
Reply to author
Forward
0 new messages