Open fingolfin opened 1 year ago
I will look into that next week.
What is the way to do such changes? The following or am I missing something important?
f
to Nemof
from HeckeIf I am not mistaken, that should leave no combination of versions, where f
is not available. There would be cases, where f
is contained twice, but that should be ok, right? Alternatively, one could circumvent that by making a breaking Nemo release such that the Nemo compat in Hecke doesn't allow the new Nemo version.
Since this is quite disruptive with little (no?) gain, I would suggest to put this on the back burner.
One gain is reduced load time for Hecke and Oscar. Isn't that a major issue for us?
It is roughly 1 second for me. Sure, 0.x (0.9? 0.6?) seconds would be great, but I believe the ratio of gain and work is quite low compared to some other open issues in Oscar land.
Another possible benefit I can see is that future changes (in particular bug fixes and interface changes) need fewer cross-project PRs. If everything (or most) functionality of a type is contained in a single package (here Nemo), there is less need for sibling PRs in Hecke and Nemo.
And I believe most of the work needed here is only simply copy-pasting the methods in the Aqua output.
It's more complicated as there are false positives...
On Fri, 16 Jun 2023, 15:35 Lars Göttgens, @.***> wrote:
Another possible benefit I can see is that future changes (in particular bug fixes and interface changes) need fewer cross-project PRs. If everything (or most) functionality of a type is contained in a single package (here Nemo), there is less need for sibling PRs in Hecke and Nemo.
And I believe most of the work needed here is only simply copy-pasting the methods in the Aqua output.
— Reply to this email directly, view it on GitHub https://github.com/thofma/Hecke.jl/issues/1126#issuecomment-1594687706, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA36CV34MGKIUTWFHUFQ7ALXLROK3ANCNFSM6AAAAAAZI5ZL7Q . You are receiving this because you are subscribed to this thread.Message ID: @.***>
It's more complicated as there are false positives...
I thought Aqua was very conservative with reporting type piracy. If there are indeed false positives, I would be very inclined to analyze them and report a fix to Aqua.
I think the point is that it is not black and white what type piracy is. Is
*(x::Union{Integer, ZZRingElem}, ::ZZRing)
in Hecke type piracy? It might be the case that this cannot be implemented in Nemo alone. So why would this constitute any form of piracy? Also it is not invalidating any other method in Nemo.
Just a small update, we are down from 721 to 316 cases, so that's great.
And yeah, we may not be able to resolve all of them for various reasons. But some maybe can...
Some things that could perhaps be moved to AA or Nemo, which would also remove some type piracy
PIDIdeal
ZZIdl
and ZZFracIdl
(most of it seems to be perfectly suitable for Nemo?)spectrum
, eigvals
Then there are still many small convenience functions that probably could be moved, e.g.:
[100] (R::fpPolyRing)(f::fqPolyRepPolyRingElem) @ Hecke ~/Projekte/OSCAR/Hecke.jl/src/NumField/NfAbs/Elem.jl:480
[101] (A::fqPolyRepField)(x::zzModPolyRingElem) @ Hecke ~/Projekte/OSCAR/Hecke.jl/src/Misc/FiniteField.jl:22
[102] (A::fqPolyRepField)(x::fpPolyRingElem) @ Hecke ~/Projekte/OSCAR/Hecke.jl/src/Misc/FiniteField.jl:31
or
#TODO: move elsewhere?
function Hecke.lcm(a::Vector{<:RingElem})
if length(a) == 0
error("don't know the ring")
end
return reduce(lcm, a)
end
etc.
Enabling the type piracy report in
test/Aqua.jl
currently reports 721 (!) instances of type piracy (with Julia 1.9). That's really bad, and might explain partially of why loading Hecke is comparatively slow.UPDATE 2024-05-08: we are down to 298 instances with Julia 1.10.3 and 1e5806b858ca0d7d776097decfda1a964b8218f7
UPDATE 2024-11-29: we are back up to 340 instances with Julia 1.11.1 and Hecke v0.34.8
I think many of these come from methods that really should be in Nemo, so perhaps we can start migrating some more. Perhaps @lgoettgens is interested in this?
Here is the list: