#1312: metadata for read-only arrays #2173

mo-lottieturner · 2023-06-12T14:25:32Z

.

…ad of myself

codecov · 2023-06-19T15:04:28Z

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (b393e04) 0.00% compared to head (fe4baf9) 99.84%.

Files	Patch %	Lines
...psyclone/domain/lfric/kernel/array_arg_metadata.py	94.00%	3 Missing ⚠️
...ne/domain/lfric/kernel/common_meta_arg_metadata.py	92.30%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #2173       +/-   ##
===========================================
+ Coverage        0   99.84%   +99.84%     
===========================================
  Files           0      353      +353     
  Lines           0    47511    +47511     
===========================================
+ Hits            0    47438    +47438     
- Misses          0       73       +73

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

mo-lottieturner · 2023-06-29T14:00:05Z

(all tests pass except codecov)

…ion for writing an array-specific version rather than a generic version.

…ight date

…nto 1312_metadata_for_arrays

mo-lottieturner · 2024-06-11T13:11:28Z

I think I've removed all of the NRANKS mentions, back to @arporter for review

arporter

Thanks for those changes @mo-lottieturner. The docs are looking pretty good now although I realise that the section describing the detailed rules for the subroutine arguments of a kernel also need to be updated.
I've now gone through the code changes but not the test files. Generally speaking it looks good although you'll see that I'm suggesting (another?) name change for clarity as I keep getting confused about distinguishing an 'array' from a 'field'. We'll need to agree any name change with at least @TeranIvy and whoever else she thinks might care!

arporter · 2024-06-13T08:57:15Z

doc/user_guide/dynamo0p3.rst

+operator <lfric-cma-operator>`. All of them except the field vector are 
+represented in the above example. ``qr`` represents a quadrature object which
+provides information required by a kernel to operate on fields (see section
+:ref:`dynamo0.3-quadrature` for more details).


Sphinx complains about this link:

user_guide/dynamo0p3.rst:116: WARNING: undefined label: 'dynamo0.3-quadrature'

Probably you're just a victim of renaming that's happened since you did this change and it just needs to be reverted back to lfric-quadrature?

arporter · 2024-06-13T10:15:10Z

doc/user_guide/dynamo0p3.rst

+Array
+++++
+
+In the LFRic API a scalar array represents a multi-valued Fortran array of


It may yet become clear to me (or I might remember) but what is a "multi-valued Fortran array"? Do you mean "multi-dimensional" or perhaps something else?

arporter · 2024-06-13T10:26:32Z

doc/user_guide/dynamo0p3.rst

-fourth is an operator. The third entry is a field vector of size 3.
+As an example, the following ``meta_args`` metadata describes 5
+entries, the first is a scalar, the second is an array, the next two
+are fields and the fifth is an operator. The third entry is a field vector


"...fourth entry is a field..."

arporter · 2024-06-13T10:28:23Z

doc/user_guide/dynamo0p3.rst

@@ -1193,16 +1210,17 @@ For example::
          does not yet support ``integer`` and ``logical`` reductions.

 For a scalar, the argument metadata contains only these three entries.
-However, fields and operators require further entries specifying
+However, arrays, fields and operators require further entries specifying


I'm guessing that the extra info for an array is unrelated to function space? If so I would delete the remainder of the sentence after "...require further entries."

arporter · 2024-06-13T10:29:59Z

doc/user_guide/dynamo0p3.rst

-subsection :ref:`lfric-function-space`.
+In the case of an operator, the fourth and fifth arguments describe the ``to``
+and ``from`` function spaces respectively. In the case of a field the fourth
+argument specifies the function space that the field lives on. In the case of


Please could you move the sentence about array to the start of this paragraph so as to preserve the function-space related flow into the last sentence.

arporter · 2024-06-13T13:35:32Z

src/psyclone/domain/lfric/lfric_arg_descriptor.py

@@ -658,7 +761,7 @@ def function_space(self):
        depending on the argument type: a single function space for a field,
        function_space_from for an operator and nothing for a scalar.

-        :returns: function space relating to this kernel argument or \
+        :returns: function space relating to this kernel argument or
                  None (for a scalar).


"scalar or ScalarArray)."

arporter · 2024-06-13T13:37:05Z

src/psyclone/domain/lfric/lfric_arg_descriptor.py

@@ -695,6 +800,8 @@ def function_spaces(self):
        if self._argument_type in const.VALID_OPERATOR_NAMES:
            # Return to before from to maintain expected ordering
            return [self.function_space_to, self.function_space_from]
+        if self._argument_type in const.VALID_ARRAY_NAMES:
+            return [self.function_space]


Should be empty list. Alternatively, could merge this and the next clause like so:

if self._argument_type in const.VALID_ARRAY_NAMES + const.VALID_SCALAR_NAMES: return []

(and ditto for the previous one too)

arporter · 2024-06-13T13:37:39Z

src/psyclone/domain/lfric/lfric_arg_descriptor.py

+    @property
+    def array_ndims(self):
+        '''
+        Returns the array size of the argument. This will be 1 if ``*n``


s/size/rank/

Oh, I see where the "default value of 1" comes from now. Do we need this method to support arguments of type other than ArrayScalar though?

arporter · 2024-06-13T13:39:18Z

src/psyclone/domain/lfric/lfric_arg_descriptor.py

+        has not been specified for all argument types except scalars
+        (their array size is set to 0).
+
+        :returns: array size of the argument.


s/size/rank/

arporter · 2024-06-13T13:40:40Z

src/psyclone/domain/lfric/lfric_constants.py

@@ -153,6 +158,11 @@ def __init__(self):
        LFRicConstants.VALID_FIELD_INTRINSIC_TYPES = ["real", "integer",
                                                      "logical"]

+        # Valid intrinsic types for array kernel argument data
+        # ('real', 'integer', and 'logical').


You don't need this line :-)

…nto 1312_metadata_for_arrays

…, and a pylint fix

mo-lottieturner and others added 10 commits June 12, 2023 15:24

#1312: starting on adding the metadata for arrays

a9f4f6f

#1312 missed a name change oops

721e7eb

#1312 list in test file needed to be updated

694dd10

Merge branch 'master' into 1312_metadata_for_arrays

fffce7e

#1312 adding tests, some not yet working

f866583

#1312 making more tests work and adding LFRicArrayArgs

f18c0eb

#1312 more test changes

e240d71

#1312 more changes for tests

eb1e9fc

#1312 removing lfric_array_args and relevant tests as was getting ahe…

6285ad1

…ad of myself

Merge branch 'master' into 1312_metadata_for_arrays

a79104a

mo-lottieturner and others added 11 commits June 20, 2023 13:58

#1312 adding metadata in the new style

705d38e

#1312 changes in new metadata

c3f45a5

#1312 removing unneeded commmented out code

bc4bbc0

#1312 changing get_vector_length to get_array_dimension

7ca546c

#1312 test for incorrect operator added

32db8f5

#1312 tidying up mistakes and pylint

aeebec2

Merge branch 'master' into 1312_metadata_for_arrays

d54e65b

#1312 temp hopeful fix for get_array_dimension

8fb098a

1312 reverting changes to get_array_dimension

5972289

#1312: *facepalm*

a1c0774

#1312 more facepalming (should work now!)

f07e02a

mo-lottieturner and others added 7 commits June 29, 2023 15:00

Merge branch 'master' into 1312_metadata_for_arrays

0c2bcb8

#1312 writing more tests

dca477d

#1312 returning get_array_dimensions to get_vector_length in preparat…

9ee4fe0

…ion for writing an array-specific version rather than a generic version.

#1312 removing name from modifier list and reinstating previous copyr…

cc0ca8b

…ight date

#1312 cleaning up pylint &c

c9e7357

Merge branch 'master' into 1312_metadata_for_arrays

2e442ed

Merge branch 'master' into 1312_metadata_for_arrays

961c519

mo-lottieturner and others added 5 commits May 30, 2024 16:47

Merge branch 'master' into 1312_metadata_for_arrays

038e0d6

#1312 line length is 79 characters not 80

8f98f7f

Merge branch '1312_metadata_for_arrays' of github.com:stfc/PSyclone i…

5240679

…nto 1312_metadata_for_arrays

#1312 removing unused ParseError

bedf340

Merge branch 'master' into 1312_metadata_for_arrays

51358d3

mo-lottieturner added ready for review and removed reviewed with actions labels Jun 11, 2024

arporter added under review and removed ready for review labels Jun 12, 2024

Merge branch 'master' into 1312_metadata_for_arrays

8acfa80

arporter requested changes Jun 13, 2024

View reviewed changes

arporter added reviewed with actions and removed under review labels Jun 13, 2024

mo-lottieturner and others added 14 commits June 18, 2024 17:37

#1312 review changes on user guide

7134c95

Merge branch 'master' into 1312_metadata_for_arrays

be295e5

#1312 lfric_constants review comment

288f7b2

Merge branch 'master' into 1312_metadata_for_arrays

8c82811

#1312 changing gh_array to gh_scalar_array

7d03e9a

Merge branch '1312_metadata_for_arrays' of github.com:stfc/PSyclone i…

e8bbc73

…nto 1312_metadata_for_arrays

#1312 ArrayArgMetadata -> ScalarArrayArgMetadata, some review changes…

62c648d

…, and a pylint fix

#1312 review changes in lfric_arg_descriptor

b22d45e

#1312 pycodestyle change

8f88377

#1312 forgot to change a test to match

21f2914

#1312 more changes from review comments

88c5135

Merge branch 'master' into 1312_metadata_for_arrays

6ad8c10

#1312 pylinting

9434c8d

#1312 more pylinting

dbfb013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#1312: metadata for read-only arrays #2173

#1312: metadata for read-only arrays #2173

mo-lottieturner commented Jun 12, 2023 •

edited

Loading

codecov bot commented Jun 19, 2023 •

edited

Loading

mo-lottieturner commented Jun 29, 2023

mo-lottieturner commented Jun 11, 2024

arporter left a comment

arporter Jun 13, 2024

arporter Jun 13, 2024

arporter Jun 13, 2024

arporter Jun 13, 2024

arporter Jun 13, 2024

arporter Jun 13, 2024

arporter Jun 13, 2024

arporter Jun 13, 2024

arporter Jun 13, 2024

arporter Jun 13, 2024

arporter Jun 13, 2024

arporter Jun 13, 2024

#1312: metadata for read-only arrays #2173

Are you sure you want to change the base?

#1312: metadata for read-only arrays #2173

Conversation

mo-lottieturner commented Jun 12, 2023 • edited Loading

codecov bot commented Jun 19, 2023 • edited Loading

Codecov Report

mo-lottieturner commented Jun 29, 2023

mo-lottieturner commented Jun 11, 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

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

mo-lottieturner commented Jun 12, 2023 •

edited

Loading

codecov bot commented Jun 19, 2023 •

edited

Loading