-
Notifications
You must be signed in to change notification settings - Fork 160
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
Remove lib/norad.gi with NormalizerViaRadical code #2282
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2282 +/- ##
=========================================
+ Coverage 73.13% 73.33% +0.2%
=========================================
Files 482 481 -1
Lines 246496 245820 -676
=========================================
+ Hits 180265 180266 +1
+ Misses 66231 65554 -677
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not remove this code! It does no harm and is good code for certain uses.
This is a good normalizer routine (in fact the only one for matrix groups with composition tree) and is also competitive for many permutation groups. (There is material back in the forum in which calling this routine was the only way to have GAP succeed with certain hard normalizer calculations.
The reason it is not called yet by Normalizer
is the question of heuristics when to call it, and the potentially high cost on lots of code if picking bad heuristics.
I will add a method for Normalizer
that assumes existence of fitting free setup but ranks lower than permutation groups -- this way the method gets called for matrix groups once FittingFreeLiftSetup
(from the matgrp
package) has been called for them.
@hulpke No worries, I did not plan to remove this code (hence the fat "DO NOT MERGE" label), except in the (unlikely) case that you said to do it (and even then, this PR would not remove it, only move it). Thing is, this code was addd in September 2014 and has been unused since then, i.e. is for all purposes dead. If it will be hooked up to the library "soon" (after being dormant for multiple years), that's great. But as long as it is not, I'd rather see it in a separate folder, or even a package. |
@fingolfin How much tolerance do we have for calculations becoming slower because the heuristics are not optimal? How are we going to find out about such cases? |
First off: I am not 100% sure I get the direction of your question correctly: do you plan to hook this up via a heuristic, and want do discuss how to do that best? Or do you want to express that you feel a heuristic is a bad idea, and really want something else? (E.g. simply expose In the first case, if you really want to discuss strategies on how to arrive at a stable heuristic: What I'd do is (a) make it easy for end users to override it and switch explicitly to one or the other approach; and (b) add tons of test cases, ideally using a kind of "A/B-testing". Here (a) is also important for the test cases, so that we can compare examples with both methods, as the heuristics are fine tuned. Then, I'd look into working on a tool that helps us automatically gather statistics on the test cases; i.e. measure all cases wth both methods; and record which method the heuristic chooses, and whether it's the "right" one (right being: the faster one, if there is a big difference; else either is fine; this can even be turned into an aggregate square: sum up the squares of the timing difference between faster method and chosen method). Then one can use this to determine how (1) improvements to either method impact performance in each test cases, and (2) how changes in the heuristic impact which method is chosen (and whether it gets "better" overall or not). I really put an emphasis on automation here, since a lot of test cases are needed to make this meaningful; I'd expect that one would make a change, and then "tell" a server somewhere to run this overnight, and the next day one decides whether one likes the outcome or not. (Whether this is run on pull requests, or triggers it manually somewhere etc. are secondary details). This is an approach I've thought about before for certain GAP features, but so far did not invest time in working on this. E.g. for the And perhaps you had something completely different in mind; I am absolutely open to other ideas, too. |
This code is undocumented and not called by anything in GAP, i.e. dead.
I've made |
Closing this, we'll take PR #2360 instead. |
(Actually, we don't remove it, we put the code into
dev/deleted-code
).This code is undocumented and not called by anything in GAP, i.e. dead. In particular, it is not tested in any way, and this has been the state since it was added in 2014 by @hulpke.
That said, it looks rather useful and functional. I just experimented a bit with it, and, ignoring the cost for the initial setup of the fitting free "framework", it seems to be indeed quite competitive! Hence I'd of course prefer if instead of removing it, we could actually use it, and/or document it. Or perhaps it should be put into a package?
But no matter what we do, first @hulpke should comment :-).