On 7/4/24 16:41, Waldek Hebisch wrote:
Opinions? Should I try to come up with a proper fix? I somehow
feel such a change will have quite some consequences in different
part of FriCAS.

Impact should be limited. Functions in AlgebraicManipulations are mostly "user level" functions. It seems that 'ratDenom' and 'rootSimp' are called from rest of algebra.

OK, then I propose the attached patch. Admittedly, I have not looked
deep into the Expression related stuff, so I hope that someone more
familiar with Expression looks over it.

The basic idea of the change I have already analysed in an earlier
message. There is the variable lv of kernels of the input expression the
tower of kernels with nthRoot as operator.

One call to root_factor_k basically only changes the leaves of the
kernel tree. So if there are nested roots the unchanged value would be
substituted back during eval.

This patch changes it such that first step by step every element of lv
get the evaluations with the elements from before.
This, of course, assumes that "rootkernels tower x" returns the kernels
from the leaves to the root.
Glancing at the implementation of SortedCache, tells me that this is
might be always true. Is that correct?

IMO we should get rid of calls to 'rootSimp' (instead extend normalization to handle algebraic kernels).

Yes. The more we have properly specified code, the better. rootSimp
sounds like an unspecifiable function.

As you noticed 'ratDenom' may be unreliable, so probably we should replace is by something else in most of algebra.

That would be good. The reason why I am currently looking into it is
that I want some kind of normal form for AlgebraicNumber. In my
computations there appear two of them in different form and I would like
to be able to verify that they are equal (and, of course, it looks
better, if the output is visually small). I don't need to solve every
case but nested sqrt and nthRoot(..., 3) would already be a good progress.

Concerning 'rootSplit', it make sense to create better implementation.

It seems that rootFactor actually also covers rootSplit. But probably
rootSplit was supposed to do things without factorization.
Anyway, if you agree with the way I "solve" the problem, I can look at
the other cases.

My impression is that original AlgebraicManipulations just used very simple tricks to get functionality similar to what other CAS-es offerd. But it was very limited.

Yes. But I think, if we agree that AlgebraicManipulations should stay at
top-level, i.e., convenience functions for users and not otherwise used
in the algebra library, then we should make those functions as
convenient as possible to assist the user in transforming expressions in
the way they want them.

There are now some extentions and enhancements, but it is still
limited.

What do you mean here?

Ralf

--
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 view this discussion on the web visit 
https://groups.google.com/d/msgid/fricas-devel/6122ca06-a5ce-4e37-8a89-89c8a2b009db%40hemmecke.org.
From c611faecae52a487873e907705a529a27cc66bc6 Mon Sep 17 00:00:00 2001
From: Ralf Hemmecke <[email protected]>
Date: Thu, 4 Jul 2024 17:38:11 +0200
Subject: fix rootFactor function to apply recursively

rootFactor did not apply the transformations recursively inside root
expressions

This patch assumes that in rootkernels tower x the kernels are sorted
by height. Smaller height must come first.

before commit:

%%% (1) -> rootFactor((y + sqrt((z+1)/(z-1))))

         +-----+     +-----+
        \|z + 1  + y\|z - 1
   (1)  --------------------
               +-----+
              \|z - 1
                                        Type: Expression(Integer)
%%% (2) -> rootFactor(sqrt(y + sqrt((z+1)/(z-1))))

         +------------+
         | +-----+
         | |z + 1
   (2)   | |-----  + y
        \|\|z - 1
                                        Type: Expression(Integer)

%%% (3) -> rootFactor(log(sqrt 6)*sqrt(sqrt(6)))
                       +----+
              +-+ +-+  | +-+
    (3)  log(\|2 \|3 )\|\|6

after commit:

%%% (4) -> rootFactor(sqrt(y + sqrt((z+1)/(z-1))))

         +--------------------+
         | +-----+     +-----+
         |\|z + 1  + y\|z - 1
   (4)   |--------------------
         |       +-----+
        \|      \|z - 1
                                        Type: Expression(Integer)

%%% (5) -> rootFactor(log(sqrt 6)*sqrt(sqrt(6)))

                      +--------+
             +-+ +-+  | +-+ +-+
   (5)  log(\|2 \|3 )\|\|2 \|3
                                        Type: Expression(Integer)
---
 src/algebra/manip.spad | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/algebra/manip.spad b/src/algebra/manip.spad
index 37b6b36b..30bd4699 100644
--- a/src/algebra/manip.spad
+++ b/src/algebra/manip.spad
@@ -369,7 +369,14 @@ AlgebraicManipulations(R, F) : Exports == Implementation where
 
           rootFactor x ==
               lk := rootkernels tower x
-              eval(x, lk, [root_factor_k k for k in lk])
+              empty? lk => x
+              lv := [root_factor_k k for k in lk]
+              rv := [first lv]
+              rk := [first lk]
+              while not empty?(lk:=rest(lk)) and not empty?(lv:=rest(lv)) repeat
+                  rv := concat(rv, eval(first lv, rk, rv))
+                  rk := concat(rk, first lk)
+              eval(x, rk, rv)
 
       if R has RadicalCategory then
         rootKerSimp(op, x, n) ==
-- 
2.34.1

Reply via email to