#2711 DoF Kernel Code Generation #2712

oakleybrunt · 2024-09-16T14:09:28Z

Closes #2711 which has a checklist of actions taken

codecov · 2024-09-16T14:24:41Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.86%. Comparing base (e615b2b) to head (70150d3).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2712   +/-   ##
=======================================
  Coverage   99.86%   99.86%           
=======================================
  Files         354      354           
  Lines       48910    48933   +23     
=======================================
+ Hits        48846    48869   +23     
  Misses         64       64

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

… 2711-dof-kernels-code

…ce test

arporter

A good first step @oakleybrunt, thank you. You'll see that I think we can do away with the new property you've added and thus shorten the code and tests somewhat.

In reading the Developer Guide, I also discovered that it needs updating - please see https://psyclone-dev.readthedocs.io/en/latest/APIs.html#loop-iterators and update it slightly.

I'll launch the integration tests next time round but I don't anticipate any issues.

arporter · 2024-09-20T08:54:26Z

doc/user_guide/dynamo0p3.rst

-   can be provided to the Kernel within a loop over the DoFs of a field.
+6) Kernels must be written to operate on a single DoF, such that field values
+   at the same dof location/index can be provided to the Kernel within a loop
+   over the DoFs of given fields.


Not yours but "...over the DoFs of the function space of the field that is being updated."

arporter · 2024-09-20T08:55:39Z

doc/user_guide/dynamo0p3.rst

@@ -1897,8 +1892,7 @@ and their interpretation are summarised in the following table:
 operates_on  Data passed for each field/operator argument
 ===========  =========================================================
 cell_column  Single column of cells
-dof          Single DoF (currently :ref:`built-ins` only, but see PSyclone
-             issue #1351)
+dof          Single DoF (also used for :ref:`built-ins`)


I think you can drop the bit in brackets. You also need to update the sentence at L1887 - possibly by just removing it entirely.

arporter · 2024-09-20T08:57:17Z

doc/user_guide/dynamo0p3.rst

-      this argument is simply ``undf`` without a function space suffix (as for
-      general purpose kernels) since all fields will be on the same function
-      space.
+      this argument is ``"undf_"<field_function_space>``.


I don't think this should be changed - since we iterate over dofs in a function space, all arguments must be on the same function space still?

arporter · 2024-09-20T09:00:32Z

src/psyclone/domain/lfric/kern_call_arg_list.py

+            # If dof kernel, add access to the field by dof ref
+            dof_sym = self._symtab.find_or_create_integer_symbol(
+                "df", tag="dof_loop_idx")
+            dof_sym = Reference(dof_sym)


This changes the type of dof_sym so that it is no longer a Symbol. I'd do away with this line and simply put [Reference(dof_sym)] directly in the argument list for append_array_reference.

arporter · 2024-09-20T09:01:41Z

src/psyclone/domain/lfric/kern_call_arg_list.py

@@ -620,6 +629,10 @@ def fs_compulsory_field(self, function_space, var_accesses=None):
            sym = self.append_array_reference(map_name, [":", ":"],
                                              ScalarType.Intrinsic.INTEGER)
            self.append(sym.name, var_accesses, var_access_name=sym.name)
+        elif self._kern.is_dofkern:
+            # Dofmaps are not compatible with user-defined DoF kernels, so


"Dof-maps are not required for DoF kernels."

arporter · 2024-09-20T09:53:22Z

src/psyclone/tests/domain/lfric/dofkern_test.py

+    assert expected in code
+
+
+def test_upper_bound_dofowned():


Hopefully you can merge this test with the previous one by using the dist_mem fixture.

arporter · 2024-09-20T09:54:51Z

src/psyclone/tests/domain/lfric/dofkern_test.py

+    expected = ("CALL testkern_dofs_code("
+                "f1_data(df), f2_data(df), f3_data(df), f4_data(df)")
+
+    assert expected in code


Please could you add a compilation check (https://psyclone-dev.readthedocs.io/en/latest/working_practises.html#compilation-testing) here as well. (Do a grep -ri compile in the tests directory to see examples.)

arporter · 2024-09-20T09:55:35Z

src/psyclone/tests/domain/lfric/dofkern_test.py

+    code = str(psy.gen)
+
+    expected = ("CALL testkern_dofs_code("
+                "f1_data(df), f2_data(df), f3_data(df), f4_data(df)")


For robustness, please could you extend the test kernel so that it takes a read-only scalar argument as well.

arporter · 2024-09-20T09:57:01Z

src/psyclone/tests/dynamo0p3_stubgen_test.py

-            "['cell_column'] but found 'domain' in kernel "
-            "'testkern_dofs_code'." in str(excinfo.value))
+            "['cell_column', 'dof'] but found 'domain' in kernel "
+            "'testkern_domain_code'." in str(excinfo.value))


If you are expecting this to work now, please could you add an explicit test for stub generation of a kernel which operates on dofs. If it isn't supported then that's fine but please reinstate this check and the appropriate bit of code.

arporter · 2024-09-20T10:01:53Z

src/psyclone/tests/domain/lfric/dofkern_test.py

+                "      loop0_stop = f1_proxy%vspace%get_last_dof_owned()")
+
+    assert expected in code
+


I think we also need to test the behaviour when 'annexed dofs' is switched on too. Again, there's an annexed fixture for this. See https://psyclone-dev.readthedocs.io/en/latest/APIs.html#dof-iterators for more details.

oakleybrunt · 2024-09-20T15:00:49Z

@arporter Can you have a look at the failing build test please? It is repeatedly tripping over on "Run examples" complaining about ACCEnterData not being able to find any data to copyin. I haven't touched any examples so don't think this is related to my changes... but I'd like your assessment, please.

arporter · 2024-09-23T09:40:50Z

src/psyclone/domain/lfric/kern_call_arg_list.py


-        self.psyir_append(Reference(sym))
+        if self._kern.iterates_over == "dof":
+            # If dof kernel, add access to the field by dof ref


Hi @oakleybrunt, I think your problem is here. If you look at the failing example it's because it's failing to get any fields to upload to the GPU. I think this is because your modification here (which admittedly I've reviewed once) is only listing the dof loop symbol, not the field. This method should probably just revert to what it was originally.

We should therefore have a new test for this method too.

oakleybrunt · 2024-10-07T15:33:54Z

@arporter Did I hear right this morning that it I should ignore stub generation for now?

arporter · 2024-10-07T15:42:53Z

@arporter Did I hear right this morning that it I should ignore stub generation for now?

Yep, you should just make sure that you have a test that we refuse to generate a stub for such a kernel.

oakleybrunt · 2024-10-09T10:13:59Z

@arporter, I can't get Codecov to recognize that I am checking the "uncovered" line (see in src/psyclone/tests/domain/lfric/dofkern_test.py::test_function_space_bare_undf). I'm stuck and asking for some help please.

arporter · 2024-10-09T12:13:27Z

@arporter, I can't get Codecov to recognize that I am checking the "uncovered" line (see in src/psyclone/tests/domain/lfric/dofkern_test.py::test_function_space_bare_undf). I'm stuck and asking for some help please.

Unfortunately, you're a victim of the fact that DynFunctionSpaces is an old bit of the code and we don't have any unit tests for it directly. I think what you need to do is create a new test: use one of your new test examples and use get_invoke() to get hold of the invoke containing the dof kernel. You can then walk the Schedule to find the kernel. That will give you a valid kernel object which you can then give to the DynFunctionSpaces constructor.

oakleybrunt added 2 commits September 16, 2024 15:06

#2711 Loops now use undf_name if dofkern

6d7cfb7

#2711 Lint fix

10f8a76

oakleybrunt and others added 8 commits September 19, 2024 11:43

#2711 dof access added

5885e78

Merge branch 'master' into 2711-dof-kernels-code

84774d0

Merge branch '2711-dof-kernels-code' of github.com:stfc/PSyclone into…

4c4a631

… 2711-dof-kernels-code

#2711 Lint fix

ec80a29

#2711 More testing for dofkern

d429e90

#2711 Lint fix

66bbc6e

#2711 LFRicBuiltin property is_dofkern return False

a8a06fd

#2711 Retracted docs specifying undf would be bare if dof kern

39c0211

oakleybrunt self-assigned this Sep 19, 2024

oakleybrunt added 4 commits September 19, 2024 15:24

#2711 Remove DoF Kernel notes from docs and add test for multiple writes

0cadb92

#2711 Docstring updates

c118a76

#2711 Rename is_dofkern in LFRicKernMetadata to match LFRicKern

a91f9a4

#2711 Missed is_user_dofkern switch and updated invalid_iteration_spa…

a688572

…ce test

oakleybrunt added ready for review LFRic Issue relates to the LFRic domain labels Sep 19, 2024

oakleybrunt requested a review from TeranIvy September 19, 2024 15:47

Merge branch 'master' into 2711-dof-kernels-code

32457c5

arporter added under review and removed ready for review labels Sep 20, 2024

arporter requested changes Sep 20, 2024

View reviewed changes

arporter added reviewed with actions and removed under review labels Sep 20, 2024

oakleybrunt and others added 5 commits September 20, 2024 15:41

Â#2711 undf name and documentation comments

23d0278

#2711 Removed is_dofkern property

56be42e

#2711 Better testing

ef8225b

#2711 Lint fix

762fb27

Merge branch 'master' into 2711-dof-kernels-code

3ba13e6

#2711 Compilation test and missed is_dofkern

b673896

arporter reviewed Sep 23, 2024

View reviewed changes

oakleybrunt and others added 16 commits October 7, 2024 16:46

#2711 reference outside

223f963

Merge branch 'master' into 2711-dof-kernels-code

76155a3

#2711 Lint fix

681f39a

#2711 append symbol

3f87b2e

#2711 drop logging for dev

19a6178

Merge branch 'master' into 2711-dof-kernels-code

77fbb5f

#2711 Reverted unknown change to nlayers name

f636ce8

#2711 Reinstate stubgen invalid iteration space test

0b2f4a3

#2711 Fix testing

c305422

#2711 Extra check on dof access for fields in kerncallarglist

a0e8598

#2711 Lint fix

be372f2

#2711 remove stub gen leftovers

8622d20

#2711 Lint fix

81ef95c

#2711 Remove stubgen LFRicFields dofkern

16361d6

#2711 Test for DynFunctionSpaces generates correct undf_name

67e8fa3

#2711 test undf DynFunctionSpaces

47f1a5e

oakleybrunt added 2 commits October 9, 2024 13:40

#2711 Another test for coverage

2704972

#2711 Fixed failing test in dofkern_test.py

70150d3

oakleybrunt requested a review from arporter October 9, 2024 13:24

oakleybrunt added ready for review and removed reviewed with actions labels Oct 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#2711 DoF Kernel Code Generation #2712

#2711 DoF Kernel Code Generation #2712

oakleybrunt commented Sep 16, 2024 •

edited

Loading

codecov bot commented Sep 16, 2024 •

edited

Loading

arporter left a comment

arporter Sep 20, 2024

arporter Sep 20, 2024

arporter Sep 20, 2024

arporter Sep 20, 2024

arporter Sep 20, 2024

arporter Sep 20, 2024

arporter Sep 20, 2024

arporter Sep 20, 2024

arporter Sep 20, 2024

arporter Sep 20, 2024

oakleybrunt commented Sep 20, 2024

arporter Sep 23, 2024

arporter Sep 23, 2024

oakleybrunt commented Oct 7, 2024

arporter commented Oct 7, 2024

oakleybrunt commented Oct 9, 2024

arporter commented Oct 9, 2024

		" loop0_stop = f1_proxy%vspace%get_last_dof_owned()")

		assert expected in code

#2711 DoF Kernel Code Generation #2712

Are you sure you want to change the base?

#2711 DoF Kernel Code Generation #2712

Conversation

oakleybrunt commented Sep 16, 2024 • edited Loading

codecov bot commented Sep 16, 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

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

oakleybrunt commented Sep 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oakleybrunt commented Oct 7, 2024

arporter commented Oct 7, 2024

oakleybrunt commented Oct 9, 2024

arporter commented Oct 9, 2024

oakleybrunt commented Sep 16, 2024 •

edited

Loading

codecov bot commented Sep 16, 2024 •

edited

Loading