(Closes #1957) Added an Intrinsic2CodeTrans metatransformation by LonelyCat124 · Pull Request #3577 · stfc/PSyclone

LonelyCat124 · 2026-09-01T09:57:40Z

Simple additional metatransformation to cleanup the nemo script.

LonelyCat124 · 2026-09-01T10:51:25Z

Should be ready now for a first look from @arporter or @sergisiso . I had a name conflict which I changed, so this transformation does have MetaTrans in its name - happy to change the name if the reviewer has a better idea.

codecov · 2026-09-01T10:53:31Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (3dd19f4) to head (8a211c2).

Additional details and impacted files

@@            Coverage Diff            @@
##            master     #3577   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          399       400    +1     
  Lines        56173     56196   +23     
=========================================
+ Hits         56173     56196   +23

☔ 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.

arporter

Thanks Aidan, I think this is a good simplification to the user interface. If @hiker and @sergisiso agree with my naming suggestion, it may be best to do that first as a separate PR as files and test files will need renaming/moving.

A few strange files have appeared in examples/nemo/scripts and probably shouldn't be there?

Finally, please update the UG section on transformations to include this functionality.

arporter · 2026-09-01T14:06:55Z

@@ -0,0 +1,33 @@
+module stringop


Is this file supposed to be included? Similarly there a out with just whitespace changes?

Oops, I think I did git add . as I would for the src dir and ended up with bonus files, I'll make sure to remove them.

arporter · 2026-09-01T14:07:46Z

@@ -0,0 +1,33 @@
+MODULE stringop


I don't think we should have a .f90 file in the nemo/scripts directory?

arporter · 2026-09-01T14:08:45Z

-                    Minval2LoopTrans().apply(intr, verbose=True)
-                elif intr.intrinsic.name == "PRODUCT":
-                    Product2LoopTrans().apply(intr, verbose=True)
+                Intrinsic2CodeMetaTrans().apply(intr, verbose=True)


arporter · 2026-09-01T14:12:20Z

+
+
+@transformation_documentation_wrapper
+class Intrinsic2CodeMetaTrans(Transformation):


Regarding naming, if @sergisiso and @hiker agree, I would suggest re-naming the existing Intrinsic2CodeTrans to Intrinsic2CodeBaseTrans (or SingleIntrinsic2CodeTrans or similar) and then having this one as Intrinsic2CodeTrans as it's the one we'd expect people to use.

I agree with this suggestion, but another alternative is to use something like: AllArrayIntrinsics2Code or something that make more clear that this transformation is aiming to loopify intrinsics that apply to all items of the array.

arporter · 2026-09-01T14:15:37Z

+
+    # Create a map of intrinsic names to the appropriate Intrinsic2Code
+    # transformation.
+    intrinsic_to_trans = {"MAXVAL": Maxval2LoopTrans,


Rather than use strings, can we use members of the IntrinsicCall.Intrinsic enum instead?

Also, we have a few more: abs2code, matmul2code, ... is it a problem for NEMO is we add them all here?

I'm not sure - I just mirrored the behaviour in the NEMO scripts, I think it would be better to do a separate performance analysis to check?

I guess these intrinsics are all array -> scalar result intrinsics, I could do a base one (this one) and nanother ArrayToScalarIntrinsics2Code or similar that just does these 4? I think we can in theory only subclass by overriding intrinsic_to_trans, but there may be some funny doc/options that appear in the docstring or are accepted but would be ignored.

arporter · 2026-09-01T14:16:19Z

+
+        # If the intrinsic is one of the supported intrinsics then
+        # apply the relevant transformation.
+        if node.intrinsic.name in Intrinsic2CodeMetaTrans.intrinsic_to_trans:


If we can use the members of the IntrinsicCall.Intrinsic enum as keys, we can remove the various .name here.

arporter · 2026-09-01T14:17:57Z

+        # apply the relevant transformation.
+        if node.intrinsic.name in Intrinsic2CodeMetaTrans.intrinsic_to_trans:
+            Intrinsic2CodeMetaTrans.intrinsic_to_trans[node.intrinsic.name]().\
+                apply(node, **kwargs_dict[node.intrinsic.name])


We could add some logging here. I'm never sure what an appropriate level of detail would be but I'm thinking that, for debugging purposes, it might be good if a user could discover that a given intrinsic has not been transformed because this transformation doesn't support it?

arporter · 2026-09-01T14:20:42Z

    "OMPCriticalTrans",
    "MaximalOMPParallelRegionTrans",
    "OMPParallelTrans",
+    "Intrinsic2CodeMetaTrans",


I think that having this __all__ list in this __init__.py file results in the annoying situation where the AutoAPI docs will create an empty page for e.g. Intrinsic2CodeMetaTrans (and all the others here). If you could put an __all__ at the end of the appropriate source file instead the that seems to work better.

arporter · 2026-09-01T14:25:03Z

+            "but received 'int'." in str(err.value))
+
+
+@pytest.mark.parametrize("code, expected", [


I think all we need to test here is that the appropriate apply method is being called - the unit tests for each individual transformation obviously cover their functionality. You might be able to do that by monkeypatching the apply method on the class so that it raises an exception and then checking for that.

(You'd need to monkeypatch the instance of the individual transfomration classes that are stored within the meta class.)

@arporter I'm not sure I quite understand, does this not test that anyway? Or I guess the point is if we change the functionality/output from the other tests we break this test as an unintended side-effect and we shouldn't care about the actual result in the tests for the MetaTransformation as they're an implementation detail of the sub transformations?

arporter · 2026-09-01T14:27:15Z

-                        del first_dict[key]
+                        # Sometimes we may have the same option in multiple
+                        # subtransformations, so we only delete the key
+                        # from the first_dict if its still present.


Added an Intrinsic2CodeTrans metatransformation

11412cf

LonelyCat124 requested review from arporter and sergisiso September 1, 2026 09:57

LonelyCat124 added the ready for review label Sep 1, 2026

LonelyCat124 added 3 commits September 1, 2026 11:10

fix a bug when multiple subtransforamtions have the same option

69d76e6

Merge branch 'master' into 1957_intrinsic2code_metatransformation

ae9f554

Fixed name clash

8a211c2

arporter added under review and removed ready for review labels Sep 1, 2026

arporter requested changes Sep 1, 2026

View reviewed changes

arporter added reviewed with actions and removed under review labels Sep 1, 2026



		@transformation_documentation_wrapper
		class Intrinsic2CodeMetaTrans(Transformation):

		"but received 'int'." in str(err.value))


		@pytest.mark.parametrize("code, expected", [

Uh oh!

Conversation

LonelyCat124 commented Sep 1, 2026

Uh oh!

LonelyCat124 commented Sep 1, 2026

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

arporter 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!

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!

LonelyCat124 Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Sep 1, 2026 •

edited

Loading

LonelyCat124 Sep 1, 2026 •

edited

Loading