On Sep 3, 2012, at 1:37 AM, Juha Jeronen <juha.jero...@jyu.fi> wrote:
> > On 31/08/12 17:52, Aaron Meurer wrote: >> On Aug 31, 2012, at 12:54 AM, Juha Jeronen <juha.jero...@jyu.fi> wrote: >> >>> Hi, >>> >>> On 30/08/12 02:43, Aaron Meurer wrote: >>>> On Wed, Aug 29, 2012 at 1:36 AM, Juha Jeronen <juha.jero...@jyu.fi> 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 :) Well it is occasionally, but as I said generally it *should* be True by default. By the way, what distro has the old SymPy. Is it an issue of you using some "stable" version, or should we try to work with the maintainers to get it upgraded? > > >>> 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). Yes, None is a common special value for cases like this. > > (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? I'm not sure. Maybe you could take a look at where in the library code collect is used to see an idea of some use cases. You could also get an idea by replacing the default and seeing how many tests fail. > > > 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). Use a pull request on Github. We'll help you if you're new to git. Aaron Meurer > > >>> 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 sympy@googlegroups.com. > To unsubscribe from this group, send email to > sympy+unsubscr...@googlegroups.com. > For more options, visit this group at > http://groups.google.com/group/sympy?hl=en. > -- You received this message because you are subscribed to the Google Groups "sympy" group. To post to this group, send email to sympy@googlegroups.com. To unsubscribe from this group, send email to sympy+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/sympy?hl=en.
