Ralf Hemmecke wrote:
>
> it looks like
>
> https://github.com/fricas/fricas/commit/25ebb18781cfe2c21f053a8a30086b66cd3724f3#diff-bcfc7730b623452ab2a4689c03f7923aL294
>
> is the first part of your patch where you actually only refactored the
> code and the second part starts further below at line 317
>
> https://github.com/fricas/fricas/commit/25ebb18781cfe2c21f053a8a30086b66cd3724f3#diff-bcfc7730b623452ab2a4689c03f7923aL317
>
> Can you confirm?
Yes, exactly.
> I just try to understand the first part of the change in the lift? function.
>
> The original code says...
>
> 1- leadpol : Boolean := false
> 2- (listpol, lval) := (uterm.lpol, uterm.lint.first)
> 3- d := listpol.first
> 4- listpol := listpol.rest
> 5- nolift : Boolean := true
> 6- for uf in listpol repeat
> 7- --note uf and d not necessarily primitive
> 8- degree gcd(uf, d) =0 => nolift := false
> 9- nolift => ["notCoprime"]
> 10- f : SUPP := ([p1, p2]$List(SUPP)).(position(uf, listpol))
> 11- lgcd := gcd(leadingCoefficient p1, leadingCoefficient p2)
> 12- (l := lift(f,d,uf,lgcd,lvar,ldeg,lval)) case "failed"=>["failed"]
> 13- [l :: SUPP]
>
> The new code is:
>
> 1+ (listpol, lval) := (uterm.lpol, first(uterm.lint))
> 2+ d := first(listpol)
> 3+ listpol := rest(listpol)
> 4+ uf := listpol(1)
> 5+ f := p1
> 6+ --note uf and d not necessarily primitive
> 7+ if degree gcd(uf, d) ~= 0 then
> 8+ uf := listpol(2)
> 9+ f := p2
> 10+ if degree gcd(uf, d) ~= 0 then return ["notCoprime"]
> 11+ lgcd := gcd(leadingCoefficient p1, leadingCoefficient p2)
> 12+ l := lift(f, d, uf, lgcd, lvar, ldeg, lval)
> 13+ l case "failed" => ["failed"]
> 14+ [l :: SUPP]
>
> Removing 1- is clear.
> The following lines are equivalent.
> 2- and 1+, 3- and 2+, 4- and 3+, 12- and (12+,13+), 13- and 14+.
>
> Then obviously both versions assume that uterm.lpol is a list that
> contains at least one element.
>
> I haven't yet checked the context whether this is safe to assume. Do you
> know?
uterm.lpol has always 3 members. Look at 'chooseVal' where the
values is computed. The first one is univariate GCD, the
other 2 are cofactors.
> Actually the old code also assumes that the list has at least length 2.
> Because otherwise uf has no value in 10-. In fact, the old code is bad
> anyway, since I consider that the loop variable is no longer available
> after the loop.
>
> Also I would miss a 'break' in the loop, because the first case where
>
> degree gcd(uf, d) =0
>
> can abort the loop. With '=>' just the rest of the body is skipped and
> the next iteration starts. For that reason in the old code, uf was
> always listpol.(#listpol).
No. When '=>' is the only thing in body then it exits the loop.
That is different from normal Spad behaviour, so at first we both
misunderstood what it was doing. At some moment is the future
we will probably change Spad compiler to avoid this special
treatment of single '=>' in a sequence, so it is better to
rewrite code depending on this behaviour.
> Without investigating what the
> input conditions are, I cannot really claim that I've found a bug. At
> least the old code and the new code are not equivalent, so the first
> part of your patch is not just refactoring it, but also a change in the
> semantics of this function.
The code is equivalent given initial conditions. #listpol different
than 3 does not make sense for this function.
--
Waldek Hebisch
[email protected]
--
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.
