Skip to content

Conversation

@fingolfin
Copy link
Member

@stevelinton removed the references to this in a commit on 2001-10-26, which (among other things) did this:

diff --git a/src/vecffe.c b/src/vecffe.c
index 30cfaf774f..96a308d354 100644
--- a/src/vecffe.c
+++ b/src/vecffe.c
@@ -1109,10 +1109,6 @@ static Int InitKernel (
     Int                 t1;
     Int                 t2;
 
-#ifdef XTNUMS
-    IsXTNumListFuncs[ T_PLIST_FFE    ] = IsXTNumPlistFFE;
-    IsXTNumListFuncs[ T_MAT_FFE      ] = IsXTNumMatFFE;
-#endif
 
     /* install the arithmetic operation methods                            */
     for ( t1 = T_PLIST_FFE; t1 <= T_PLIST_FFE + IMMUTABLE; t1++ ) {
@@ -1133,11 +1129,6 @@ static Int InitKernel (
         }
     }
 
-#ifdef XTNUMS
-    for ( t1 = T_PLIST_FFE; t1 <= T_PLIST_FFE + IMMUTABLE; t1++ ) {
-        ProdFuncs[ t1        ][ T_MAT_FFE ] = ProdVecFFEMatFFE;
-    }
-#endif
     
     InitHdlrFuncsFromTable( GVarFuncs );
 

The alternative to dropping this code would to be to figure out if it is correct, then export it as a GAP level function and install it suitably as a method for \*. But nobody seems to have missed it all those years, so I am not sure it'd be worth the trouble... I'd much rather see that time and energy invested in documenting the MatrixObj/VectorObj APIs, and then writing "new" compressed matrix and vector code which drops the legacy need for being compatible with list APIs...

@fingolfin fingolfin added topic: kernel release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Jan 20, 2019
@fingolfin fingolfin requested a review from stevelinton January 20, 2019 16:25
@codecov
Copy link

codecov bot commented Jan 20, 2019

Codecov Report

Merging #3203 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3203      +/-   ##
==========================================
+ Coverage   84.76%   84.77%   +<.01%     
==========================================
  Files         687      687              
  Lines      335859   335822      -37     
==========================================
  Hits       284690   284690              
+ Misses      51169    51132      -37
Impacted Files Coverage Δ
src/vecffe.c 77.74% <ø> (+6.72%) ⬆️

@coveralls
Copy link

Coverage Status

Coverage increased (+0.008%) to 84.652% when pulling 5ea68ec on fingolfin:mh/ProdVecFFEMatFFE into 2648e28 on gap-system:master.

@stevelinton
Copy link
Contributor

The function was "orphaned" because the kernel didn't have a way to tell if it was applicable. Working out whether a given list was a matrix of FFEs wasn't done as part of the type determination and I think it was once done in an extended part of that (hence the references to xtnum).

The function could be exported to GAP level and used by callers who knew for sure what their matrices were. A quick experiment suggests that this is actually a little slower (not sure why) but does allocate a lot less memory. It could be useful in the matrix obj setup where the methods will (sometimes) know that they that they have a matrix stored as a vector of equal-length FFEs. On the other hand if they really care about performance they should be using cvec or meataxe64.

So on balance I'd approve this, I think, but I'll leave it open for discussion a bit longer

@fingolfin
Copy link
Member Author

@stevelinton well, this code is for lists-of-FFEs and lists-of-lists-of-FFEs; I'd suggest that people who want it fast first should turn to compressed vectors and matrices. The code also was dead for at least 17 years, without nobody complaining about it missing. From my POV, trying to expose or integrate it just isn't worth the effort. I also don't see how it'd be useful for the MatrixObj effort; there, I'd expect we'd write new, more efficient functions taking its place.

@stevelinton
Copy link
Contributor

It's use, if it has one, is for field orders between 257 and the limit of the built-in FFEs (2^16 or 2^24).
The built-in compressed vectors don't handle those cases, although cvec and meataxe64 do. I have no objection to it being deleted.

@fingolfin
Copy link
Member Author

@stevelinton OK, good -- could you then please approve this PR?

@fingolfin fingolfin merged commit dc843a6 into gap-system:master Jan 28, 2019
@fingolfin fingolfin deleted the mh/ProdVecFFEMatFFE branch January 28, 2019 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: kernel

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants