#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 (59668bb) to head (681581b).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2712   +/-   ##
=======================================
  Coverage   99.86%   99.86%           
=======================================
  Files         354      354           
  Lines       48963    48980   +17     
=======================================
+ Hits        48899    48916   +17     
  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.

doc/user_guide/dynamo0p3.rst

src/psyclone/domain/lfric/kern_call_arg_list.py

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

src/psyclone/tests/dynamo0p3_stubgen_test.py

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

arporter · 2024-10-11T14:09:36Z

Is there a possibility that when ACC transformations happen, the KernCallArgList is remade with no checks for where the arguments originate from? I'm not entirely sure what's happening

I think the sub-class KernCallACCArgList is used to work out what data needs to be transferred to the GPU. Perhaps this method gets called by that class?

oakleybrunt · 2024-10-16T12:05:52Z

I think the sub-class KernCallACCArgList is used to work out what data needs to be transferred to the GPU. Perhaps this method gets called by that class?

That is exactly what's happening. Does this mean that the isinstance check is justified, or that another change needs to be made?

arporter · 2024-10-16T12:23:14Z

That is exactly what's happening. Does this mean that the isinstance check is justified, or that another change needs to be made?

It probably means that you need to override this method in KernCallACCArgList and make sure that it provides the correct list of arguments for an ACC enter data directive. For a field, this will just be its name, irrespective of whether or not the kernel operates on cell columns or dofs (which is different from the kernel argument that this method in this class is generating).

arporter

Looking very nice now Oakley, thanks. Well done for minimising the code changes.
Some of the new methods you've added need explicit tests. (We have a rule that the test battery associated with a given source file should give 100% coverage of that file. In some places we're still working towards this ambition but all new code should have appropriate tests added.)
I've also realised that we probably need more 'functional' tests as there's lots that can go wrong when adding an important new concept like this. Please could you add a test that Dynamo0p3RedundantComputationTrans (https://psyclone-ref.readthedocs.io/en/latest/_static/html/classpsyclone_1_1transformations_1_1Dynamo0p3RedundantComputationTrans.html) works when applied to one of the new dof kernels.
You'll need to check that the halo-exchanges are to the correct depth.
Please could you also construct a more complex test invoke that mixes kernels that operate on cell-columns, user kernels that operate on dofs and, finally, builtin kernels.
You could base this off e.g. test_files/dynamo0p3/3.1_multi_functions_multi_invokes.f90 but you needn't bother with multiple invokes.
In the meantime, I've also fired off the integration tests but it's going to take a while for them to run.

arporter · 2024-10-18T13:57:43Z

src/psyclone/domain/lfric/kern_call_acc_arg_list.py

@@ -93,6 +93,27 @@ def cell_position(self, var_accesses=None):
        _, ref = self.cell_ref_name(var_accesses)
        self.append(ref.symbol.name)

+    def field(self, arg, var_accesses=None):


Please add a test for this method to tests/domain/lfric/kern_call_acc_arg_list_test.py

arporter · 2024-10-18T14:05:09Z

src/psyclone/domain/lfric/lfric_kern.py

@@ -557,6 +557,19 @@ def base_name(self):
        '''
        return self._base_name

+    @property
+    def undf_name(self):


Please add a test for this method in tests/domain/lfric/lfric_kern_test.py.

arporter · 2024-10-18T14:10:19Z

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

+    assert 'undf' in test_fs._var_list
+
+
+def test_compiles(tmpdir):


This test can go now.

arporter · 2024-10-18T14:33:03Z

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

+    assert LFRicBuild(tmpdir).code_compiles(psy)
+
+
+def test_undf_initialisation(tmpdir):


This test can probably go now unless it is covering something specific to dof kernels.

arporter · 2024-10-18T14:36:36Z

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

+                    "      loop0_stop = f1_proxy%vspace%get_last_dof_owned()"
+                    )
+
+    # Shared memory


Annexed only has any effect if distributed-memory is enabled so you can simplify this to elif not dist_mem:

arporter · 2024-10-18T16:11:08Z

Unfortunately the compilation checks failed:

Looks like you just need proper declarations for the arguments to the test kernel.

You can test this locally by doing pytest --compile ....

oakleybrunt · 2024-10-21T12:46:50Z

As I am working on making the dof kernel (src/psyclone/tests/test_files/dynamo0p3/testkern_dofs_mod.f90) compile, I noticed again that the arguments passed to the subroutine are single dofs, real or integer. This makes me question why we are passing undf to the kernel call at all. Originally, I thought it would be used for creating an array (e.g dimension(undf)), but I don't see a use for it here, much like it isn't passed to LFRic built-ins.

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

oakleybrunt added 2 commits October 11, 2024 13:34

#2711 Update dev guide

7240a09

#2711 Lint fix

5a9e09c

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

2e024e9

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

c835cc7

oakleybrunt added 4 commits October 16, 2024 13:37

#2711 Add field method to KernCallAccArgList

0464e5a

#2711 Restore append method for field symbol

8f09184

#2711 Add undf_name property to LFRicKern

28ad8e6

#2711 Lint Fix

8df39d0

oakleybrunt requested a review from arporter October 17, 2024 15:55

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

arporter added under review and removed ready for review labels Oct 18, 2024

arporter temporarily deployed to integration October 18, 2024 14:40 — with GitHub Actions Inactive

arporter requested changes Oct 18, 2024

View reviewed changes

arporter added reviewed with actions and removed under review labels Oct 18, 2024

#2711 Added testing

42ee334

oakleybrunt and others added 4 commits October 22, 2024 09:48

#2711 Remove undf from kern args, transformation testing

682bfff

#2711 Lint fixes

cacc273

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

0b80e3d

#2711 Testing transformations and multi-kernels

681581b

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

oakleybrunt requested a review from arporter October 22, 2024 12:57

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 commented Oct 11, 2024

oakleybrunt commented Oct 16, 2024

arporter commented Oct 16, 2024

arporter left a comment

arporter Oct 18, 2024

arporter Oct 18, 2024

arporter Oct 18, 2024

arporter Oct 18, 2024

arporter Oct 18, 2024

arporter commented Oct 18, 2024 •

edited

Loading

oakleybrunt commented Oct 21, 2024 •

edited

Loading

		assert 'undf' in test_fs._var_list


		def test_compiles(tmpdir):

		assert LFRicBuild(tmpdir).code_compiles(psy)


		def test_undf_initialisation(tmpdir):

#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

arporter commented Oct 11, 2024

oakleybrunt commented Oct 16, 2024

arporter commented Oct 16, 2024

arporter left a comment

Choose a reason for hiding this comment

arporter Oct 18, 2024

Choose a reason for hiding this comment

arporter Oct 18, 2024

Choose a reason for hiding this comment

arporter Oct 18, 2024

Choose a reason for hiding this comment

arporter Oct 18, 2024

Choose a reason for hiding this comment

arporter Oct 18, 2024

Choose a reason for hiding this comment

arporter commented Oct 18, 2024 • edited Loading

oakleybrunt commented Oct 21, 2024 • edited Loading

oakleybrunt commented Sep 16, 2024 •

edited

Loading

codecov bot commented Sep 16, 2024 •

edited

Loading

arporter commented Oct 18, 2024 •

edited

Loading

oakleybrunt commented Oct 21, 2024 •

edited

Loading