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.
