On 9/10/15, Ralf Hemmecke <[email protected]> wrote: > On 09/10/2015 07:32 AM, Abhinav Baid wrote: >>> PS: I'll try to produce a patch to change Set to List in lodof2.spad. >>> >> >> I just did that. [1] >> There was a Set FA in the code too, so I changed that also. >> >> [1] >> https://github.com/fandango-/fricas/commit/81ca9a8067d94c1287dd98f4dec3df02b8e199ac >> >> >> >> Thanks, >> Abhinav. > > Thank you. And yes, now the Aldor stuff compiles, but I wonder why it > already works. > > Aldor should probably have complained about this statement as well. > > https://github.com/fricas/fricas/blob/master/src/algebra/lodof2.spad#L281 > > semi : Table(FA, UP) := table() > > since we have > > FA ==> Record(factor : UP, exponent : Integer) > > and > > https://github.com/fricas/fricas/blob/master/src/algebra/table.spad#L99 > > Table(Key : SetCategory, Entry : Type) : Exports == Implementation where > > > > Anyway, Abhinav, I'd like to make some comments to your code. > > https://github.com/fricas/fricas/blob/master/src/algebra/lodof2.spad#L284 > > for i in v for ii in 1..(#v)-1 repeat > restl := rest(v, ii) > > This looks somewhat ugly to me. Since v is a list of factors, one can > assert that it's never the empty list, so something like > > i := first v > while not empty?(v := rest v) repeat > -- Your code here with 'restl' replaced by just 'v'. > -- There shouldn't be '=> "iterate"' then, because it would > -- miss the following statement. > -- Since in other places of this loop you also do not use '=>' > -- to restart iteration, for me it feels like some change in > -- coding style that makes the code harder to read. > i := first v > > 'rest(v, ii)' unnecessarily introduces linear time. > I've changed it to the tails form as suggested by Waldek and also removed the '=>' used.
> As for the variable 'semi'. It is only important if 'option="semireg". > Feels to me like you are wasting time computing 'semi' it if this option > is not set. > > Furthermore, in case of "semireg", you use 'semi' as a Table to have > direct access to semi.i or semi.j. But do your really need it? The > current implementation of Table falls back to AssociationList for your > Key type and that has linear time access. Maybe better avoid Table. > I've changed it to a OneDimensionalArray. [1] > In the end you remove all the unsafe factors from v. I've now not looked > at the paper, but it looks a bit suspicious that your code can put i > several times into the 'unsafe' list. That means at the same time that > the factor i is added to semi(j) for two different j's. > > Furthermore, it can happen that one j puts i into the 'unsafe' list and > the same i (although now "unsafe") puts a later j to the 'unsafe' list. > Is that behaviour OK? Yes, the above points are non-issues. [1] https://github.com/fandango-/fricas/commit/429c736a867205ecab54cb17026d8384a09850ec -- Abhinav. -- You received this message because you are subscribed to the Google Groups "FriCAS - computer algebra system" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. Visit this group at http://groups.google.com/group/fricas-devel. For more options, visit https://groups.google.com/d/optout.
