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 code "converting" T_PERM{2,4} to Julia arrays #137

Merged
merged 1 commit into from
Nov 6, 2018

Conversation

fingolfin
Copy link
Member

This code was untested, and seems of dubious use to me: Instead, if somebody
needs to do this, they should be able to simply use ListPerm, followed by a
conversion from plist to array.

If that is not fast enough because, say, someone need to convert a million
permutations, then with overwhelming probably they are doing something very
wrong. But if a legitimate use case pops up, we can still add back similar
code. Although I still would not want to have it in the "generic"
ConvertedToJulia function; rather it should be in a dedicated
ConvertGAPPermToJuliaArray function.

This code was untested, and seems of dubious use to me: Instead, if somebody
needs to do this, they should be able to simply use `ListPerm`, followed by a
conversion from plist to array.

If that is not fast enough because, say, someone need to convert a million
permutations, then with overwhelming probably they are doing something very
wrong. But if a legitimate use case pops up, we can still add back similar
code. Although I still would not want to have it in the "generic"
`ConvertedToJulia` function; rather it should be in a dedicated
`ConvertGAPPermToJuliaArray` function.
@fingolfin fingolfin requested a review from sebasguts November 6, 2018 10:35
@codecov
Copy link

codecov bot commented Nov 6, 2018

Codecov Report

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

@@            Coverage Diff             @@
##           master     #137      +/-   ##
==========================================
+ Coverage   54.28%   54.54%   +0.25%     
==========================================
  Files          44       44              
  Lines        2590     2574      -16     
==========================================
- Hits         1406     1404       -2     
+ Misses       1184     1170      -14
Impacted Files Coverage Δ
JuliaInterface/src/JuliaInterface.c 92.49% <ø> (+4.14%) ⬆️

@sebasguts sebasguts merged commit 0d17b08 into oscar-system:master Nov 6, 2018
@fingolfin fingolfin deleted the mh/no-T_PERM-conversion branch November 6, 2018 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants