On 31/08/12 17:52, Aaron Meurer wrote: > On Aug 31, 2012, at 12:54 AM, Juha Jeronen <[email protected]> wrote: > >> Hi, >> >> On 30/08/12 02:43, Aaron Meurer wrote: >>> On Wed, Aug 29, 2012 at 1:36 AM, Juha Jeronen <[email protected]> wrote: >>>> Or should we add a new API function, keeping rcollect() as-is? This >>>> solution >>>> doesn't seem very clean. >>> Well, I actually would like to have just one function, collect(), but >>> that would require changing the API. So I would try to find a good >>> name for each strategy, and use method="name" in rcollect(). As to >>> what should be the default, I don't know. >> If we don't want to break the API, I think the current method of >> rcollect() should be the default (since otherwise the behaviour of the >> function would change). > If it will make it cleaner, we should consider breaking the API, with > a deprecation on the old one if possible.
Ok. >> Maybe >> >> method="depth-first" vs. method="breadth-first"? >> >> Difficult to type, especially breat...breadh...aaaaahhh! How about > bfs and dfs are common abbreviations for these. Ok. Those are fine by me. "dfs" for the current rcollect(), and "bfs" for the suggested recursive_collect(). >> method="sums" vs. method="all"? (Additionally, method="top" could be >> top-level-only, i.e. the current behaviour of collect().) > method="all" sounds like "apply all the methods". Hmm, you're right. So that was a bad choice :) > Top-level only is exactly what deep=False usually does. Mm, yes. So it's better to do it with that to be consistent. >> In addition to this, we could add a flag to the existing collect(), >> which would make it behave as rcollect(). Default would be off to avoid >> breaking the API. This way, the user could use the current rcollect() >> API or the new extended collect() API. >> >> As an added bonus, this would be consistent with other simplifiers, >> which have exactly this behaviour: same function for recursive and >> non-recursive, with "deep" off by default. > In my experience, deep is on by default. Also in my opinion in most > cases it should be, as this is what you want most of the time. Ok. I'm not sure why I thought it is off by default - might be in some functions in the old 0.6.7. As I'm using both 0.7.1 and 0.6.7 at the moment (two machines, different distro), I may be a bit confused about API details at times :) >> E.g. deep=True, and if this was enabled, the kwarg "method" would become >> available (just like the suggested change for rcollect), and collect() >> would basically call rcollect(), passing through the value for "method". >> Then rcollect() could internally use collect() in the non-recursive form. >> >> (Or maybe split collect() into a pure API function and an internal >> worker function to avoid the circular-looking use relationship between >> collect() and rcollect() in the suggested solution. But that depends on >> which you think is clearer; I'm fine with either option.) > Maybe collect(deep=True) can call rcollect, which we can also > deprecate. Then, when the deprecation period ends, we just remove > rcollect from __init__.py. Ok. Summarizing the discussion, the cleanest solution is probably to change the API to: collect(expr, syms=None, **kwargs) kwargs: deep=True (default; API break!) or deep=False (top level only, like old collect()) method="dfs" (old rcollect()) or method="bfs" (recursive_collect()). For syms, a list can be given as before. The default is the special value None, which means "use automatic syms". rcollect(expr, *vars) just calls collect(expr, syms, deep=True, method="dfs"). Questions: - Is None good as a special value? I think it's at least better than the string "auto", because IIRC collect() isn't that particular about the type of syms being a list if you want to collect on one symbol only (I think this is a very nice convenience feature and should be kept). Given this behaviour, the value None is the only one that doesn't pass control information in the data band (as it's not a valid symbol). (Of course, whatever the special value is, it can be the default when syms is omitted, so the user doesn't need to worry about it.) - Which method should be the default? Personally, I would suggest the new bfs; at least it's what I intuitively expect myself when doing a "recursive collect" (i.e. "extract as much stuff to as near the top level as possible"). What is your opinion on this? And the other developers? Once these are settled, I think I have enough information to prepare a proper patch (except for practical details, such as the preferred patch format). >> Mm, yes. I did try to pass in a list to rcollect() at first (as in "ah, >> just prefix the function name with an r"), until reading the >> documentation and noticing the star :) >> >> Consistent types are of course the most important thing in this regard, >> but I think consistent parameter naming would be a nice bonus. It would >> allow the user-programmer to learn the API faster, if the same kind of >> thing always had the same name. > Agreed. This can be changed throughout SymPy without an API break (in > other words, patches welcome). Ok. I might look into this when I have the time, but no promises :) -J -- You received this message because you are subscribed to the Google Groups "sympy" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/sympy?hl=en.
