kpeeters / cadabra2

A field-theory motivated approach to computer algebra.
https://cadabra.science/
GNU General Public License v3.0
230 stars 37 forks source link

Lack of deepcopy in vary() argument. Is this intentional? Simple minimal working example. #289

Closed phelps-matthew closed 10 months ago

phelps-matthew commented 10 months ago
ex1:= A B + A C.
ex2 = vary(ex1, $A -> \epsilon D, B -> \epsilon C, C -> \epsilon A - \epsilon B$ ).
ex1;
\epsilon D B + A \epsilon C + \epsilon D C + A (\epsilon A  \epsilon B)

Why is the default behavior to alter the passed in argument of ex1? Why not use a copy so that ex1 retains its definition? This seems nonstandard.

kpeeters commented 10 months ago

This all depends on your point of view, there is something to be said for both options.

If in pure Python you do

def fun(q):
   q[1]=8

a=[1,2,3]
fun(a)
a

you get [1,8,3]. So it's not totally uncommon or non-standard.

Cadabra's logic is that the most common use case involves writing down a starting expression, and then doing a number of subsequent manipulations to arrive at a final expression. It is then quite annoying if you have to write

ex:= ...;
ex = some_function(ex)
ex = another_function(ex)
ex = third_function(ex)
...

Also keep in mind that Cadabra was originally written (back in 2001) to do a certain type of computations which could just barely fit into available memory. Making a copy of every expression (at least temporarily) would have limited what we were able to do. This is not likely to be an issue for most applications now, but it certainly was back then.

So neither of these may convince you (and that's fine), but at least there is some logic behind it all. It would probably not be too hard to make a switch to instruct Cadabra to always make a copy of the "incoming" expression, act on that, and return it, if someone really wanted that behaviour.

phelps-matthew commented 10 months ago

Thanks for the detailed response. True, performing operations on mutables in Python is definitely an acceptable thing to do for some cases, and I can understand it being more convenient here if used under sequential operations. Unfortunately, I have many use cases of doing things like:

deltaRicciTensor = vary(ricciTensor, $R^{\rho}_{\sigma\rho\nu}->\dR^{\rho}_{\sigma\rho\nu}, R_{\sigma\nu} -> \dR_{\sigma\nu}$);

This updates the definition of the ricciTensor itself so I can no longer use (or must manually reset it). I actually tried to use deepcopy, but this threw an error "RuntimeError: Free indices in different terms do not match".

I wonder what solutions are available at my disposal here?

I have also encountered similar behavior for functions beyond vary(), though when and where it applies is not very clear to me.

For reference, I am drawing from examples as in this notebook: https://cadabra.science/notebooks/einstein_equations.html

kpeeters commented 10 months ago

All built-in Cadabra algorithms act inside the given expression, not just vary. The few exceptions to this rule are things in some of the packages.

You can definitely do

from copy import deepcopy
ex1:= A B + A C;
ex2 = vary(deepcopy(ex1), $A -> \epsilon D, B -> \epsilon C, C -> \epsilon A - \epsilon B$ );

to leave ex1 intact and store the result in ex2. If you have an expression for which that does not work, let me know (minimal working example please, as it probably then fails because of interplay with the properties system).

phelps-matthew commented 10 months ago

Indeed, I did find that it worked on that example. Here is a MWE for you:

from copy import deepcopy
{\mu,\nu,\rho,\sigma,\alpha,\beta,\kappa,\gamma,\lambda,\tau,\lambda#}::Indices().
\nabla{#}::Derivative.
ex := F_{\mu\nu} = \nabla_{\mu}{A_{\nu}} - \nabla_{\nu}{A_{\mu}};
deepcopy(ex)

yielding RuntimeError: Free indices in different terms in a sum do not match.

Perhaps this is centered around the Derivative property?

kpeeters commented 10 months ago

Actually, ignore the deepcopy thing. Cadabra's Ex has a copy method, so you can just do

ex1:= A B + A C;
ex2 = vary(ex1.copy(), $A -> \epsilon D, B -> \epsilon C, C -> \epsilon A - \epsilon B$ );

That should work with any Cadabra expression.

phelps-matthew commented 10 months ago

Great! I will use the built in method instead. Thank you.