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/f4d917def699f6b78a404cd89dba1248491a766f
> >>
> >>>
> >>> 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 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.
> >
> >
>
--
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.
