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.
On Tuesday, February 20, 2024 at 2:53:26 PM UTC Giacomo Pope wrote: > 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/c33194b9-6d7d-497c-ae55-040dcc04c250n%40googlegroups.com.
