(Towards #2905) mv lfric symbol creation by arporter · Pull Request #3499 · stfc/PSyclone

arporter · 2026-07-20T10:29:01Z

Move the Symbol creation into the LFRicInvoke constructor. This then affects some tests that were having to pre-populate a table.

codecov · 2026-07-20T10:41:11Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.99%. Comparing base (cd6df42) to head (f8bcff1).

❌ Your project check has failed because you have indirect coverage changes. Learn more about Unexpected Coverage Changes and reasons for indirect coverage changes.

Additional details and impacted files

@@             Coverage Diff             @@
##            master    #3499      +/-   ##
===========================================
- Coverage   100.00%   99.99%   -0.01%     
===========================================
  Files          397      397              
  Lines        55587    55569      -18     
===========================================
- Hits         55587    55568      -19     
- Misses           0        1       +1

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…one into 2905_mv_lfric_symbol_creation

arporter · 2026-07-22T13:04:44Z

A relatively simple PR that modernises some of the PSy-layer generation.
The LFRic ITs have passed.
One for @sergisiso @LonelyCat124 or @hiker :-)

LonelyCat124

Hi @arporter I think I mostly understand this, have a couple of questions and a couple of small fixes requested, and one I'm not 100% on so I'd like to rerun ITs again after just in case.

LonelyCat124 · 2026-07-22T13:54:57Z

+    ) -> None:
+        '''
+        Add compulsory arguments associated with this function space to
        the list. If supplied it also stores this access in var_accesses.


Suggested change

the list. If supplied it also stores this access in var_accesses.

this argument list. If supplied it also stores this access in var_accesses.

LonelyCat124 · 2026-07-22T14:07:23Z

-                        f"integer(kind=i_def), pointer :: {dmap}(:,:) "
-                        f"=> null()"))
-                self.symtab.add(dmap_sym, tag=dmap)
+            intrinsic_type = LFRicTypes("LFRicIntegerScalarDataType")()


Certainly not one for this PR, but LFRicTypes being dependent on strings feels a bit messy somehow though I think it would require some significant rewriting and object-orientation to do much to.

Yes, LFRicTypes is a bit of an issue (e.g. #2659).

LonelyCat124 · 2026-07-22T14:12:17Z

+                    intrinsic_type,
+                    [ArrayType.Extent.DEFERRED, ArrayType.Extent.DEFERRED]))
+            dmap_sym = self.symtab.find_or_create_tag(
+              dmap, symbol_type=DataSymbol, datatype=dtype)


Small thing - if we process code like this through the Frontend:

program x integer, pointer, dimension(:, :) :: a => null() end program x

then the symbol we come across has an initial value set:

a: DataSymbol<UnsupportedFortranType('INTEGER, POINTER, DIMENSION(:, :) :: a => null()'), Unknown, initial_value=IntrinsicCall[name='NULL']>

Could you add the initial value into this find_or_create_tag to match? If this causes issues though then we probably don't have to.

I started doing this (initial_value = IntrinsicCall.create(Intrinsic.NULL)) but then realised that will create a standard assignment rather than a pointer assignment (#2577). Therefore, I don't think we can do it currently but I'll add a TODO. I've also added partial_datatype to the other sorts of dofmap that this method creates since I'm here.

See comment on Teams, I'd still like this to happen

Done now - no issues so far.

LonelyCat124 · 2026-07-22T14:16:23Z

+                         self.reference_element_properties,
+                         self.mesh_properties, self.loop_bounds,
+                         self.run_time_checks]:
+            entities.invoke_declarations()


I assume all LFRicCollections need to have their own implementation of invoke_declarations? Can you test making the one define in the LFRicCollections base class abstractmethod to ensure we can't miss it in subclasses?

They do but the base one is not empty - it contains a check that the LFRicInvoke has been setup correctly.

Yeah, thats not an issue, you can still call super().func() up to an abstractmethod, see this example:

import abc class base: @abc.abstractmethod def func(self): print("Hello from base") class sub(base): def func(self): print("Hello from sub") super().func() sub().func()

Aha! Done now.

LonelyCat124 · 2026-07-22T14:24:14Z

+            self,
+            loop: Loop,
+            test_all_variables: bool = False,
+            signatures_to_ignore: Optional[Signature] = None) -> bool:


I did a bit of looking at this, and its another thing we'll need to update post-3.9, Optional is a bit of a misnomer, which means "can be None" and not "optional". As of 3.10 we should use Signature | None for typehints like this, I added a comment to #3416

LonelyCat124 · 2026-07-22T14:26:01Z

-                                        parallelised.
+        :param test_all_variables: if True, it will test if all variable
+            accesses can be parallelised, otherwise it will stop after the
+            first variable is found that can not be parallelised.


Can this docstirng be rewritten slightly to remove the need to specify "if True" - since its a bool I think we can do something like "whether to continue to test if all variable accesses can be parallelised once a variable has been found that can not be parallelised"?

arporter · 2026-07-23T10:29:37Z

Thanks @LonelyCat124, ready for another look now (CI permitting).

LonelyCat124

I've left a couple of things still to be resolved from last time as I think they are both possible and would improve the code.

LonelyCat124 · 2026-07-23T11:36:02Z

+                    intrinsic_type,
+                    [ArrayType.Extent.DEFERRED, ArrayType.Extent.DEFERRED]))
+            dmap_sym = self.symtab.find_or_create_tag(
+              dmap, symbol_type=DataSymbol, datatype=dtype)


See comment on Teams, I'd still like this to happen

LonelyCat124 · 2026-07-23T11:42:24Z

+                         self.reference_element_properties,
+                         self.mesh_properties, self.loop_bounds,
+                         self.run_time_checks]:
+            entities.invoke_declarations()


Yeah, thats not an issue, you can still call super().func() up to an abstractmethod, see this example:

import abc class base: @abc.abstractmethod def func(self): print("Hello from base") class sub(base): def func(self): print("Hello from sub") super().func() sub().func()

arporter · 2026-08-06T17:51:51Z

Took me a while to get back to this but should be ready for another look now.

arporter added 5 commits July 14, 2026 13:07

#3398 add to_clean option to transformation

07e4aa1

#3398 mv the LFRic dofmap symbol creation to the Invoke constructor

344529b

#3398 more work to fix tests [skip ci]

5a0c552

#3398 fix all tests

01bd743

Merge branch 'master' into 3398_redundant_to_clean_depth

28ee48f

arporter marked this pull request as draft July 20, 2026 10:29

arporter self-assigned this Jul 20, 2026

arporter and others added 2 commits July 20, 2026 11:41

#2905 make generated code consistent with previous and revert changes

e5b1890

Merge branch 'master' into 2905_mv_lfric_symbol_creation

111c969

arporter added in progress LFRic Issue relates to the LFRic domain labels Jul 20, 2026

arporter added 3 commits July 22, 2026 13:07

#3499 rm unused code

6286991

Merge branch '2905_mv_lfric_symbol_creation' of github.com:stfc/PSycl…

cd08d45

…one into 2905_mv_lfric_symbol_creation

Merge branch 'master' into 2905_mv_lfric_symbol_creation

f9e91a7

arporter temporarily deployed to integration July 22, 2026 12:08 — with GitHub Actions Inactive

arporter marked this pull request as ready for review July 22, 2026 12:12

#2905 update cov of dependency_tools

9d51292

arporter added ready for review and removed in progress labels Jul 22, 2026

arporter requested review from LonelyCat124, hiker and sergisiso July 22, 2026 13:04

LonelyCat124 added under review and removed ready for review labels Jul 22, 2026

LonelyCat124 requested changes Jul 22, 2026

View reviewed changes

LonelyCat124 added reviewed with actions and removed under review labels Jul 22, 2026

Merge branch 'master' into 2905_mv_lfric_symbol_creation

929f7b2

#2905 tidying for review

8c5ed3a

arporter added ready for review and removed reviewed with actions labels Jul 23, 2026

LonelyCat124 requested changes Jul 23, 2026

View reviewed changes

LonelyCat124 added reviewed with actions and removed ready for review labels Jul 23, 2026

arporter added 2 commits August 6, 2026 18:26

Merge branch 'master' into 2905_mv_lfric_symbol_creation

1009dd0

#2905 make invoke_declarations abstract and add initial_type for dofmaps

f8bcff1

arporter added ready for review and removed reviewed with actions labels Aug 6, 2026

	the list. If supplied it also stores this access in var_accesses.
	this argument list. If supplied it also stores this access in var_accesses.

Uh oh!

Conversation

arporter commented Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

arporter commented Jul 22, 2026

Uh oh!

LonelyCat124 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

arporter commented Jul 23, 2026

Uh oh!

LonelyCat124 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arporter commented Aug 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

arporter commented Jul 20, 2026 •

edited

Loading

codecov Bot commented Jul 20, 2026 •

edited

Loading