-
Notifications
You must be signed in to change notification settings - Fork 66
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Type piracy in Hecke #1126
Comments
I will look into that next week. What is the way to do such changes? The following or am I missing something important?
If I am not mistaken, that should leave no combination of versions, where |
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
<#1126 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA36CV34MGKIUTWFHUFQ7ALXLROK3ANCNFSM6AAAAAAZI5ZL7Q>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
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
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
Then there are still many small convenience functions that probably could be moved, e.g.:
or
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 1e5806b
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:
The text was updated successfully, but these errors were encountered: