(Closes #2774) Refactor matmul2code to allow for more permutations of full ranges #2795

DrTVockerodtMO · 2024-11-20T13:02:17Z

There was a hardcoded standard in PSyclone where for matrix multiplications, the first one or two indices must be full ranges (i.e.: the Fortran would be written as matrix(:,:,...) or vector(:,...)). However, Fortran can perform matrix multiplications with the full ranges in different orderings, as long as the shape conforms.

When recent LFRic changes re-ordered some of the operator index accesses, this shifted the full ranges into forbidden permutations.

This change addresses this issue by allowing more permutations that produce valid Fortran matmul code.

Description of changes
Added a new private method _get_full_range_split to matmul2code_trans.py, which splits information about full range and non full range nodes as well as performs some validation. For a given array, the method iterates over the children, and checks whether or not they are Range nodes. If the Range node is a full range, we append its position in the array to a list. If not, we throw an error. If it is not a Range node then its position and the node itself are appended to two different lists.

After the children are iterated over, if we have more than two full ranges then this array is not a valid input for matmul, so we throw an error. Otherwise, return the two lists containing the positions of the full range and non full range nodes in the array, as well as the non full range node list.

Some further validation is performed to ensure that the first matrix involved in the matmul has exactly two full ranges, and that the second matrix has one or two (since it can be a vector or matrix).

The orderings and list of non full range nodes are fed to the modified _create_matrix_ref to make the matrix reference.

Testing
A new test test_apply_matmat_reordered has been added which tests the expected behaviour directly, by feeding the PSyIR some Fortran code with spatially conforming but re-ordered (with respect to the old standard) matrices. Other tests will be modified since a lot of them tested aspects of the old standard that no longer apply.

…t to get full range ordering

…e correct number of full ranges (it would always show 3 otherwise). Re-ordered filling of indices to match previous code.

codecov · 2024-11-20T13:50:51Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.88%. Comparing base (4cb5070) to head (8a5caa9).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2795   +/-   ##
=======================================
  Coverage   99.88%   99.88%           
=======================================
  Files         356      356           
  Lines       49481    49511   +30     
=======================================
+ Hits        49423    49455   +32     
+ Misses         58       56    -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

arporter

Good stuff @DrTVockerodtMO.
I think the fix/extension is good. Just a few minor things to tidy up and I've requested an extra test.
Although not strictly your fault, I note that the associated test file does not give 100% coverage - lines 424-425 are missed. You can see this by doing pytest --cov psyclone.psyir.transformations.intrinsics.matmul2code_trans -- cov-report term-missing psyir/transformations/intrinsics/matmul2code_trans_test.py. Could you take a look?
Please could you also update the class docstring of apply() to mention the restriction to full ranges and simple arrays (i.e. structures of arrays etc.).
I've fired off the integration tests so these can run in the meantime.

src/psyclone/psyir/transformations/intrinsics/matmul2code_trans.py

src/psyclone/tests/psyir/transformations/intrinsics/matmul2code_trans_test.py

arporter · 2024-11-21T11:36:19Z

src/psyclone/tests/psyir/transformations/intrinsics/matmul2code_trans_test.py

-            "multiplication, the second index of the 2nd argument 'y' must "
-            "be a full range." in str(excinfo.value))
+    assert ("Transformation Error: To use matmul2code_trans on matmul, "
+            "no more than two indices of the argument 'x' "


debug_string() again.

Addressed in 10d5abb.

Ah, I see now that I might have confused you here - apologies. I meant 'use debug_string() when creating the error message', not when testing it!

src/psyclone/tests/psyir/transformations/intrinsics/matmul2code_trans_test.py

arporter · 2024-11-21T13:31:10Z

Just a note to say integration tests were OK (barring the expected failures).

DrTVockerodtMO · 2024-11-21T15:51:48Z

Good stuff @DrTVockerodtMO. I think the fix/extension is good. Just a few minor things to tidy up and I've requested an extra test. Although not strictly your fault, I note that the associated test file does not give 100% coverage - lines 424-425 are missed. You can see this by doing pytest --cov psyclone.psyir.transformations.intrinsics.matmul2code_trans -- cov-report term-missing psyir/transformations/intrinsics/matmul2code_trans_test.py. Could you take a look? Please could you also update the class docstring of apply() to mention the restriction to full ranges and simple arrays (i.e. structures of arrays etc.). I've fired off the integration tests so these can run in the meantime.

Changes should all be addressed. Regarding not covering lines 424-425, these lines have been replaced in favour of the new method of generating array references, which are tested by the new test that was requested.

arporter · 2024-11-21T21:09:20Z

src/psyclone/tests/psyir/transformations/intrinsics/matmul2code_trans_test.py

-    assert ("To use matmul2code_trans on matmul, exactly two indices of the "
-            "1st argument 'x' must be full ranges "
-            "but found 1." in str(excinfo.value))
+    assert (f"To use {trans.name} on matmul, exactly two indices of the "


Please could you change this so that it checks that the output string is what we expect, rather than what we coded. i.e. this doesn't need to be an f-string - just hardwire in the values that we expect.

Addressed in c4d60bc.

arporter · 2024-11-21T21:09:33Z

src/psyclone/tests/psyir/transformations/intrinsics/matmul2code_trans_test.py

-    assert ("Transformation Error: To use matmul2code_trans on matmul, "
-            "one or two indices of the 2nd argument 'y' "
-            "must be full ranges but found 0." in str(excinfo.value))
+    assert (f"Transformation Error: To use {trans.name} on matmul, "


Again, no f-string here please.

Addressed in c4d60bc.

arporter · 2024-11-21T21:09:45Z

src/psyclone/tests/psyir/transformations/intrinsics/matmul2code_trans_test.py

-    assert ("Transformation Error: To use matmul2code_trans on matmul, "
-            "each Range index of the argument 'x' must be a full range "
-            "but found non full range at position 2." in str(excinfo.value))
+    assert (f"Transformation Error: To use {trans.name} on matmul, "


No f-string...

Addressed in c4d60bc.

arporter · 2024-11-21T21:09:54Z

src/psyclone/tests/psyir/transformations/intrinsics/matmul2code_trans_test.py

-    assert ("Transformation Error: To use matmul2code_trans on matmul, "
-            "each Range index of the argument 'y' must be a full range "
-            "but found non full range at position 2." in str(excinfo.value))
+    assert (f"Transformation Error: To use {trans.name} on matmul, "


No f-string...

Addressed in c4d60bc.

arporter · 2024-11-21T21:10:05Z

src/psyclone/tests/psyir/transformations/intrinsics/matmul2code_trans_test.py

-    assert ("Transformation Error: To use matmul2code_trans on matmul, "
-            "no more than two indices of the argument 'x' "
-            "must be full ranges but found 3." in str(excinfo.value))
+    assert (f"Transformation Error: To use {trans.name} on matmul, "


No f-string...

Addressed in c4d60bc.

arporter · 2024-11-21T21:10:25Z

src/psyclone/tests/psyir/transformations/intrinsics/matmul2code_trans_test.py

-    assert ("Transformation Error: To use matmul2code_trans on matmul, "
-            "no more than two indices of the argument 'y' "
-            "must be full ranges but found 3." in str(excinfo.value))
+    assert (f"Transformation Error: To use {trans.name} on matmul, "


No f-string...

Addressed in c4d60bc.

arporter · 2024-11-21T21:42:34Z

src/psyclone/tests/psyir/transformations/intrinsics/matmul2code_trans_test.py

-    assert ("To use matmul2code_trans on matmul, each range on the result "
-            "variable 'result' must be a full range but found "
-            "result(2:4,2:5)" in str(excinfo.value))
+    assert (f"To use {trans.name} on matmul, each range on the result "


"No f-string"

Addressed in c4d60bc.

src/psyclone/tests/psyir/transformations/intrinsics/matmul2code_trans_test.py

arporter · 2024-11-21T22:00:23Z

Changes should all be addressed. Regarding not covering lines 424-425, these lines have been replaced in favour of the new method of generating array references, which are tested by the new test that was requested.

That's good but those lines are still there and are still not covered? On closer examination it looks as though this code will only work for something of the form result(:,:,any,other,indices) = matmul(....) but this isn't checked in validate. If it's not too hard, do you think you could generalise this bit too? Alternatively, we should check for this situation in validate and reject it.

arporter

Thanks Terry. Apologies for the confusion over the strings (KGOs) in the tests.
Apart from that, looking at the missed lines made me realise that we still seem to have an implicit assumption in the code about the form of the result array that we support.
We can either generalise this too or we can make the assumption explicit by checking for it in validate.
The good news is that the integration tests were all fine :-)

arporter · 2024-11-21T21:47:39Z

src/psyclone/tests/psyir/transformations/intrinsics/matmul2code_trans_test.py

@@ -994,3 +998,40 @@ def test_apply_matmat_reordered(tmpdir, fortran_reader, fortran_writer):
        "    enddo\n"
        "  enddo\n" in out)
    assert Compile(tmpdir).string_compiles(out)
+
+
+def test_apply_matmat_varexpr_index(tmpdir, fortran_reader, fortran_writer):


Thanks for adding this test but note that it is actually 'matvec' and please update the doc string too - the second argument is a 1D slice.

Addressed in 4629275.

arporter · 2024-11-21T21:49:45Z

src/psyclone/psyir/transformations/intrinsics/matmul2code_trans.py

-    :returns: the new reference to a matrix element.
+    :param List[int] loop_idx_order: list of indices in the original array \
+                                     where the loop_idx_symbols are.
+


I was going to let this go but since there are other changes required, please could you rm this blank line and there's no need for the line-continuation chars (\) - we used to think they were necessary but Joerg pointed out that all we have to do is suitably indent any continued lines.

Addressed in 4629275.

DrTVockerodtMO · 2024-11-21T22:42:36Z

Thanks Terry. Apologies for the confusion over the strings (KGOs) in the tests. Apart from that, looking at the missed lines made me realise that we still seem to have an implicit assumption in the code about the form of the result array that we support. We can either generalise this too or we can make the assumption explicit by checking for it in validate. The good news is that the integration tests were all fine :-)

Addressed in 4629275 and 703c3e1 which now has a different permutation of the result matrix in the calculation.

…o test the result validation.

DrTVockerodtMO added 4 commits November 18, 2024 12:23

Initial code changes (not tidy), modified and added some tests

b7f4844

Removed _get_first function, changed behaviour of get_full_range_spli…

440c1ff

…t to get full range ordering

Placed error raising after full range order generation so it shows th…

cd52a9e

…e correct number of full ranges (it would always show 3 otherwise). Re-ordered filling of indices to match previous code.

Fixed docstrings, bug in full range split. Added preliminary test.

3d2486f

DrTVockerodtMO added bug enhancement LFRic Issue relates to the LFRic domain adjoint labels Nov 20, 2024

DrTVockerodtMO self-assigned this Nov 20, 2024

DrTVockerodtMO added 2 commits November 20, 2024 13:34

Tidied up and added new tests

b8c6ca2

Fixed flake8 linting

6ecb5f5

Merge branch 'master' into 2774_matmul2code_trans_refactor

ee0b992

DrTVockerodtMO marked this pull request as ready for review November 20, 2024 14:23

DrTVockerodtMO requested a review from arporter November 20, 2024 14:23

DrTVockerodtMO and others added 2 commits November 20, 2024 14:36

Added my name to author list that was missing from previous PR

297c6c6

Merge branch 'master' into 2774_matmul2code_trans_refactor

19bb05c

arporter added the under review label Nov 21, 2024

TeranIvy mentioned this pull request Nov 21, 2024

Change index order for LFRic operators (for # 2766) #2792

Open

arporter temporarily deployed to integration November 21, 2024 11:40 — with GitHub Actions Inactive

arporter requested changes Nov 21, 2024

View reviewed changes

arporter added reviewed with actions Release Planning and creating PSyclone releases and removed under review labels Nov 21, 2024

Addressed reviewer comments

10d5abb

DrTVockerodtMO and others added 4 commits November 21, 2024 13:31

Removed print statement from new test

a801f18

Fixed some linting

fb607c0

Fixed more linting

107cbcb

Merge branch 'master' into 2774_matmul2code_trans_refactor

6e5c2fb

DrTVockerodtMO requested a review from arporter November 21, 2024 15:21

arporter reviewed Nov 21, 2024

View reviewed changes

arporter added under review and removed reviewed with actions labels Nov 21, 2024

arporter requested changes Nov 21, 2024

View reviewed changes

arporter added reviewed with actions and removed under review labels Nov 21, 2024

DrTVockerodtMO added 4 commits November 21, 2024 22:17

Removed f-strings from KGOs in test

c4d60bc

Tidied docstrings, generalised result reference generation

4629275

Changed test to test for different permutation in result matrix

703c3e1

Removed unused local variable

b64acc6

DrTVockerodtMO and others added 5 commits November 21, 2024 22:54

Changed example so it conforms to matmul rules

9dd9f8b

Changed validation of result matrix to use same structure as matrix2

bfa526b

Fixed linting

44d0a65

Fixed error message to say 'result' over 'matrix2'. Added new tests t…

b627716

…o test the result validation.

Merge branch 'master' into 2774_matmul2code_trans_refactor

8a5caa9

DrTVockerodtMO requested a review from arporter November 21, 2024 23:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(Closes #2774) Refactor matmul2code to allow for more permutations of full ranges #2795

(Closes #2774) Refactor matmul2code to allow for more permutations of full ranges #2795

DrTVockerodtMO commented Nov 20, 2024

codecov bot commented Nov 20, 2024 •

edited

Loading

arporter left a comment

arporter Nov 21, 2024

DrTVockerodtMO Nov 21, 2024

arporter Nov 21, 2024

arporter commented Nov 21, 2024

DrTVockerodtMO commented Nov 21, 2024

arporter Nov 21, 2024

DrTVockerodtMO Nov 21, 2024

arporter Nov 21, 2024

DrTVockerodtMO Nov 21, 2024

arporter Nov 21, 2024

DrTVockerodtMO Nov 21, 2024

arporter Nov 21, 2024

DrTVockerodtMO Nov 21, 2024

arporter Nov 21, 2024

DrTVockerodtMO Nov 21, 2024

arporter Nov 21, 2024

DrTVockerodtMO Nov 21, 2024

arporter Nov 21, 2024

DrTVockerodtMO Nov 21, 2024

arporter commented Nov 21, 2024

arporter left a comment

arporter Nov 21, 2024

DrTVockerodtMO Nov 21, 2024

arporter Nov 21, 2024

DrTVockerodtMO Nov 21, 2024

DrTVockerodtMO commented Nov 21, 2024

(Closes #2774) Refactor matmul2code to allow for more permutations of full ranges #2795

Are you sure you want to change the base?

(Closes #2774) Refactor matmul2code to allow for more permutations of full ranges #2795

Conversation

DrTVockerodtMO commented Nov 20, 2024

codecov bot commented Nov 20, 2024 • edited Loading

Codecov Report

arporter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arporter commented Nov 21, 2024

DrTVockerodtMO commented Nov 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arporter commented Nov 21, 2024

arporter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DrTVockerodtMO commented Nov 21, 2024

codecov bot commented Nov 20, 2024 •

edited

Loading