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. On Tuesday, February 20, 2024 at 2:17:57 PM UTC Dima Pasechnik wrote: > Did you try running tests locally? > Our CI sometimes acts up > > > On 20 February 2024 12:49:03 GMT, Giacomo Pope <giaco...@gmail.com> wrote: > >> 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. >> >> A link to the PR is here: https://github.com/sagemath/sage/pull/37369 >> >> Thank you! >> >> -- You received this message because you are subscribed to the Google Groups "sage-devel" group. To unsubscribe from this group and stop receiving emails from it, send an email to sage-devel+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/sage-devel/9667e799-81ce-4a8c-a575-3ed0a7f7a823n%40googlegroups.com.
