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 unused code (rational classes of perm groups) #886

Merged
merged 1 commit into from
Aug 16, 2016

Conversation

fingolfin
Copy link
Member

This removes some code that was unused since at least July 2005. Back then, @hulpke commented it out in the CVS repository. Two relevant quotes by him:

Date:   2005-07-12 14:42:03 +0000

    Disabled old rational classes code, new code uses conjugacy classes. AH

and

Date:   2005-08-06 12:55:14 +0000

    Rational classes fused from classes remember rationality.
    Further removal of old RationalClasses code that seems to be broken. AH

Based on some comments in the code, as well as "git blame", I think this code was originally written by Heiko Theißen. Perhaps it is meant to be faster for computing rational classes of perm groups -- but the fact that it hasn't been used for over a decade leads me to think we are better of removing it (I spent some time today trying to understand parts of this code, until I figured out it wasn't actually used).

If anybody wants to resurrect and fix it in the future: It'll still be available in the history... :)

@codecov-io
Copy link

Current coverage is 49.13% (diff: 100%)

Merging #886 into master will increase coverage by 0.06%

@@             master       #886   diff @@
==========================================
  Files           422        422          
  Lines        228711     228396   -315   
  Methods        3447       3447          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits         112224     112227     +3   
+ Misses       116487     116169   -318   
  Partials          0          0          

Powered by Codecov. Last update 80a0966...dcff97b

@hulpke
Copy link
Contributor

hulpke commented Aug 16, 2016

@fingolfin Thank you -- the code (which is based on a paper by Butler, and on Heiko's diploma thesis) was intendd to be faster. Code degradation over 10 years led to problems .
The commenting-out was a "CVS style" solution as it is harder to get back to old versions. Removing the code is the right choice.

@olexandr-konovalov olexandr-konovalov merged commit 08a718f into gap-system:master Aug 16, 2016
@olexandr-konovalov
Copy link
Member

@fingolfin thanks! Deleted code is debugged code :)

@fingolfin
Copy link
Member Author

In some very limited tests I made, the removed code was actually slower than what we have now.

@fingolfin fingolfin deleted the mh/cleanup branch August 18, 2016 15:32
@olexandr-konovalov olexandr-konovalov added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Jan 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: added PRs introducing changes that have since been mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants