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.
0001-just-call-fcode-and-derivation-friendly-settings.patch
Description: application/mbox
