Okay these are pushed in. -- Andy
On Wed, Jul 14, 2010 at 12:57 PM, Øyvind Jensen <jensen.oyv...@gmail.com> wrote: > It's on http://github.com/jegerjensen/sympy/tree/toons_fixes > > On 14 Jul, 19:41, Øyvind Jensen <jensen.oyv...@gmail.com> wrote: >> I'll put them up. Running tests now... >> >> Øyvind >> >> on., 14.07.2010 kl. 10.10 -0700, skrev Ondrej Certik: >> >> > Toon -- if you have time, please upload them to github, then we can >> > easily pull it. >> >> > Ondrej >> >> > On Wed, Jul 14, 2010 at 9:16 AM, Andy Ray Terrel <andy.ter...@gmail.com> >> > wrote: >> > > Øyvind, >> >> > > Have these patches been added to a repo somewhere so I can push them >> > > in? I'm not sure which goes where. >> >> > > -- Andy >> >> > > On Sat, Jul 10, 2010 at 10:35 AM, Øyvind Jensen >> > > <jensen.oyv...@gmail.com> wrote: >> > >> Hi Toon, >> >> > >> Thanks for your patches/suggestions and for looking over my work! >> >> > >> lø., 10.07.2010 kl. 09.09 +0200, skrev Toon Verstraelen: >> > >>> Hi, >> >> > >>> I've been looking into the fcode and codegen implementation. Sorry for >> > >>> the delay. First of all: I'm really happy with the work being done. I >> > >>> wish I could dedicate more time to the reviewing. >> >> > >>> I have a few small patches, but consider them as suggestions. (See >> > >>> attachments.) >> >> > >>> 0001-just-call-fcode-and-derivation-friendly-settings.patch >> >> > >>> I'm more in favor of calling fcode directly. Then there is no need to >> > >>> write set_some_setting methods in the printers, which is much cleaner >> > >>> API-wise. The overhaed should be small, but I don't know what the >> > >>> actual >> > >>> reason was to have a self._printer atribute in FCodeGen? I also added a >> > >>> test to make sure that the settings are handled in an >> > >>> inheritance-friendly way. >> >> > >> I agree that the set_assign_to method is ugly, and in retrospect I see >> > >> how this could be avoided by sticking with fcode instead of >> > >> self._printer. The reason I changed to self._printer, was because I >> > >> wanted to ensure that the printer had identical settings in every call >> > >> from the FCodeGen object. In the end, it turned out that the printer is >> > >> called only at one place, so self._printer is not really necessary. >> >> > >> I propose a slight simplification of you patch: >> >> > >> + assign_to = self._settings['assign_to'] >> > >> + if isinstance(assign_to, basestring): >> > >> + self._settings['assign_to'] = Symbol(assign_to) >> > >> + elif isinstance(assign_to, (Basic, type(None))): >> > >> + self._settings['assign_to'] = assign_to >> > >> else: >> > >> raise TypeError("FCodePrinter cannot assign to object of >> > >> type %s"% >> > >> - type(result_variable)) >> > >> + type(assign_to)) >> >> > >> The elif clause doesn't do anything. This would be better: >> >> > >> + assign_to = self._settings['assign_to'] >> > >> + if isinstance(assign_to, basestring): >> > >> + self._settings['assign_to'] = Symbol(assign_to) >> > >> + elif not isinstance(assign_to, (Basic, type(None))): >> > >> raise TypeError("FCodePrinter cannot assign to object of >> >> > >> This is also implemented in the attachment. >> >> > >>> 0002-Fixed-missing-comma-in-__all__-in-codegen.py.patch >> >> > >>> This seems futile, but it is probably something that happens easily. Is >> > >>> it possible to add a code quality test that checks of 'from mod import >> > >>> *' works for every module? >> >> > >> The patch is fine, and the test you propose would be useful, but I don't >> > >> know how to implement it. >> >> > >>> 0003-Fixed-return-value-assignment-in-FCodeGen.patch >> >> > >>> Just a small bug fix. >> >> > >> Thanks, the patch is fine. The test reveals that the logic to determine >> > >> if initialization is needed, is far from perfect. But that's another >> > >> issue. >> >> > >>> Some more suggestions that came to mind: >> >> > >>> - Rename FCodeGen to F95CodeGen (also use .f95 then). In the long run >> > >>> it >> > >>> would be interesting to have an F77CodeGen too. I guess it will be >> > >>> different enough to justify two classes. >> >> > >> Good idea. >> >> > >>> - The expr argument of the Routine constructor was one of the next >> > >>> things I wanted to modify before I stopped working on codegen. Now the >> > >>> constructor tries to figure out which expression is a return variable, >> > >>> and so on. It would be more convenient to replace expr by results, i.e. >> > >>> a list of ResultBase instances. This gives some more freedom to the >> > >>> user >> > >>> and it makes the implementation way easier. The current trick with the >> > >>> Equality class is rather counterintuitive. >> >> > >> I like the simple user interface where you just supply the expression >> > >> and let the constructor figure out something that works. This is also >> > >> very useful for automatic compilation[0]. But I realize that more >> > >> control could be useful, and I think we can easily have both. >> >> > >> Given that the constructor should accept Expr instances, I don't agree >> > >> that the *treatment* of the Equality class is counter-intuitive. But if >> > >> your concern is about the Equality class being the only user interface >> > >> for obtaining code with output arguments, I see what you mean. >> >> > >> Again, thanks a lot for the patches. They are all +1 with the slight >> > >> modification of the first. >> >> > >> Cheers, >> > >> Øyvind >> >> > >> [0]: >> > >>http://github.com/jegerjensen/sympy/commit/f4d917def699f6b78a404cd89d... >> >> > >>> cheers, >> >> > >>> Toon >> >> > >> -- >> > >> You received this message because you are subscribed to the Google >> > >> Groups "sympy-patches" group. >> > >> To post to this group, send email to sympy-patc...@googlegroups.com. >> > >> To unsubscribe from this group, send email to >> > >> sympy-patches+unsubscr...@googlegroups.com. >> > >> For more options, visit this group >> > >> athttp://groups.google.com/group/sympy-patches?hl=en. >> >> > > -- >> > > You received this message because you are subscribed to the Google >> > > Groups "sympy-patches" group. >> > > To post to this group, send email to sympy-patc...@googlegroups.com. >> > > To unsubscribe from this group, send email to >> > > sympy-patches+unsubscr...@googlegroups.com. >> > > For more options, visit this group >> > > athttp://groups.google.com/group/sympy-patches?hl=en. >> >> > > -- > You received this message because you are subscribed to the Google Groups > "sympy-patches" group. > To post to this group, send email to sympy-patc...@googlegroups.com. > To unsubscribe from this group, send email to > sympy-patches+unsubscr...@googlegroups.com. > For more options, visit this group at > http://groups.google.com/group/sympy-patches?hl=en. > > -- You received this message because you are subscribed to the Google Groups "sympy-patches" group. To post to this group, send email to sympy-patc...@googlegroups.com. To unsubscribe from this group, send email to sympy-patches+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/sympy-patches?hl=en.
