Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

fingolfin
Copy link
Member

(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 :-).

@fingolfin fingolfin added do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Mar 22, 2018
@fingolfin fingolfin requested a review from hulpke March 22, 2018 14:08
@codecov
Copy link

codecov bot commented Mar 22, 2018

Codecov Report

Merging #2282 into master will increase coverage by 0.2%.
The diff coverage is n/a.

@@            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
Impacted Files Coverage Δ
src/hpc/threadapi.c 37.55% <0%> (ø) ⬆️
src/stats.c 87.19% <0%> (+0.13%) ⬆️

Copy link
Contributor

@hulpke hulpke left a 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.

@fingolfin
Copy link
Member Author

@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.

@hulpke
Copy link
Contributor

hulpke commented Mar 27, 2018

@fingolfin
As for tying the method in that it gets used also in the permutation case: I cannot imagine that any initial heuristic makes optimal decisions.

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?

@fingolfin
Copy link
Member Author

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 NormalizerViaRadical as a documented user function; or making Normalizer switch to it only when a certain option s given etc.)
In the latter case: I am happy with any way of exposing NormalizerViaRadical to users, directly or indirectly; the only thing I don't like is having this code around but unused and undocumented.

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 recog package it would be interesting to have... If you are interested in something in this vein (of course I am open to suggestions on how to tackle this differently!), I'd be happy to pitch in on this.

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.
@hulpke
Copy link
Contributor

hulpke commented Apr 13, 2018

I've made NormalizerViaRadical a proper function and added documentation. This is in an open PR now.

@fingolfin
Copy link
Member Author

Closing this, we'll take PR #2360 instead.

@fingolfin fingolfin closed this Apr 17, 2018
@fingolfin fingolfin deleted the mh/rm-norad branch April 17, 2018 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants