PSyclone icon indicating copy to clipboard operation
PSyclone copied to clipboard

Spaces introduces into real constants

Open hiker opened this issue 2 years ago • 2 comments

While working on #1883, I detected one failure:

In the tutorial/practicals/LFRic/building_code/1_simple_kernels/part2/solutions there is a -2.0_r_def in , which turns up inside KernCallArgList as - 2.0_r_def - notice the additional space! This causes Literal to fail when testing if it checks a real constant with the regular expression.

This seems not to be related to #1919 , since the same error happens when the _r_def is removed from the source code.

hiker avatar Oct 17 '22 07:10 hiker

As discussed in the telco, root cause is that in Fortran a signed real number is actually a unary-operation on the (positive) real number, and along the way to PSyclone this unary-operation is converted to a string, separated with a space:

class UnaryOpBase(Base):
    """
::
    <unary-op-base> = <unary-op> <rhs>
    """
    def tostr(self):
        return '%s %s' % tuple(self.items)

in fparser/two/utils.py. Removing this spacse causes at least the following errors to be triggered:

FAILED psyad/adjoint_visitor_test.py::test_loop_node_active[lo,hi,-1-hi - MOD(hi - lo, - 1), lo, 1] - AssertionError: assert 'do i = hi - ... 0.0\nenddo\n' == 'do i = hi - ......
FAILED parse/algorithm_test.py::test_getkernel_isliteral_expr[- 1.0-datatype0] - AssertionError: assert '-1.0' == '- 1.0'
FAILED parse/algorithm_test.py::test_getkernel_isliteral_expr[- 1-datatype1] - AssertionError: assert '-1' == '- 1'

These errors are likely easy to fix, e.g.:

--- a/src/psyclone/tests/psyad/adjoint_visitor_test.py
+++ b/src/psyclone/tests/psyad/adjoint_visitor_test.py
@@ -716,7 +716,7 @@ def test_loop_node_passive(fortran_reader):
 @pytest.mark.parametrize("in_bounds,out_bounds", [
     ("lo,hi", "hi, lo, -1"),
     ("lo,hi,1", "hi, lo, -1"),
-    ("lo,hi,-1", "hi - MOD(hi - lo, - 1), lo, 1"),
+    ("lo,hi,-1", "hi - MOD(hi - lo, -1), lo, 1"),
     ("lo,hi,step", "hi - MOD(hi - lo, step), lo, -1 * step")])
 def test_loop_node_active(fortran_reader, fortran_writer, in_bounds,
                           out_bounds):

But it might also be cleaner/better not to convert the unary-expression into a string in psyclone/parser/algorithm in get_kernel:

            arguments.append(Arg('literal', argument.tostr().lower(),
                                 datatype=datatype))

At this location argument is still a tree with '-' and a real constant as children.

hiker avatar Oct 17 '22 12:10 hiker

The solution to this is to switch over the Algortihm layer to use PSyIR: #1618.

arporter avatar Oct 17 '22 14:10 arporter

While working on the review for #1883, as suggested I replaced the explicit code with psyir_from_expression. I've added explicit tests for the space handling, and it is not causing a problem anymore, so I'll close this issue.

hiker avatar Nov 17 '22 00:11 hiker