Hi Burcin, On Thu, May 7, 2009 at 8:25 AM, Burcin Erocal <bur...@erocal.org> wrote: > I guess the first patch is a collection of my patches sitting on trac, > so I didn't read it. Is this right?
Yep. > Some minor comments after reading the 2nd patch: > > * does new_Expression_from_GEx() really need the new parent parameter? Yes, since callable symbolic expressions which you get from doing things like "f(x) = x^2" are just Expressions with different parents. This avoids having two parallel classes that we have to keep in sync. If you can think of a different solution, I'd be all for it. > * how does the new _convert() function relate to the _eval_self() I > defined to handle numerical approximations? Actually, _convert() can be deleted, it's left over from before we had _eval_self(). > * in the _factor_list() method, the line "if op is not None:" seems > superfluous Yep -- it was from the original _factor_list. I've taken care of this in my tree which is at /scratch/mhansen/sage-3.4.2.alpha0-sage.math-only-x86_64-Linux/devel/sage-symbolics > * in the initialization of SFunction, I had removed the find_function() > call, since you don't want to overwrite a previously user created > function which might be present in previously created expressions. > It seems that your patch adds it back. We need this in order to have our sin function match up with the one in GiNaC. There's a check that the end so that it only takes the serial from find_function if it isn't a builtin function in GiNaC. > * I don't think SFunction should have a .serial() method. It is useful > for debugging but it shouldn't be exposed to users. Fair enough -- it should be removed. > * can we not use from sage.all import ... in function.pyx? Not at the top level. Things such as sin, cos, etc. all use function.pyx. > * why is SR.pi() necessary? Compatibility with the old interface. > * the docstring for SR.var() is confusing, since you use it to create > multiple symbolic variables, and return expressions if the argument > is already an expression We can fix this. > * I don't see immediately why the printing functions are in the > parent, and not the elements. I.e., why is printing deferred to > SR._repr_element() and SR._latex_element()? This is for callable symbolic expressions. > As I pointed out earlier on IRC, I don't think it's necessary to patch > pynac at all for the constant evaluation. You can just pass in any > python object which implements a .numerical_approx() method (the python > object for the constant itself?) to the constant constructor. This > would also remove the need for a lookup table for numerical > approximation of constants. > > > This is the first time I saw the default_variable() function in the previous > symbolics code. I suggest that this is deprecated, and the functions that > need this require explicitly stating variables. Maybe this discussion should > take place in a different thread though, since it's independent of your patch. I believe sin(x).derivative() breaks without it. Another thread would be good. --Mike --~--~---------~--~----~------------~-------~--~----~ To post to this group, send email to sage-devel@googlegroups.com To unsubscribe from this group, send email to sage-devel-unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/sage-devel URLs: http://www.sagemath.org -~----------~----~----~----~------~----~------~--~---
