From a503be3a9a704bc88c568e448357a48e9ec8e0d6 Mon Sep 17 00:00:00 2001 From: "Sadie L. Bartholomew" Date: Fri, 19 Apr 2024 18:17:05 +0100 Subject: [PATCH 01/12] Add del_construct method to account for cyclic axis deletion --- cf/mixin/fielddomain.py | 101 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 101 insertions(+) diff --git a/cf/mixin/fielddomain.py b/cf/mixin/fielddomain.py index 85cf503e55..c85a5379a9 100644 --- a/cf/mixin/fielddomain.py +++ b/cf/mixin/fielddomain.py @@ -2408,6 +2408,107 @@ def set_construct( # Return the construct key return out + def del_construct(self, *identity, default=ValueError(), **filter_kwargs): + """Remove a metadata construct. + + If a domain axis construct is selected for removal then it + can't be spanned by any data arrays of the field nor metadata + constructs, nor be referenced by any cell method + constructs. However, a domain ancillary construct may be + removed even if it is referenced by coordinate reference + construct. + + .. versionadded:: 3.16.2 + + .. seealso:: `get_construct`, `constructs`, `has_construct`, + `set_construct` + + :Parameters: + + identity: + Select the unique construct that has the identity, + defined by its `!identities` method, that matches the + given values. + + Additionally, the values are matched against construct + identifiers, with or without the ``'key%'`` prefix. + + {{value match}} + + {{displayed identity}} + + default: optional + Return the value of the *default* parameter if the + data axes have not been set. + + {{default Exception}} + + {{filter_kwargs: optional}} + + .. versionadded:: (cfdm) 1.8.9.0 + + :Returns: + + The removed metadata construct. + + **Examples** + + >>> f = {{package}}.example_field(0) + >>> print(f) + Field: specific_humidity (ncvar%q) + ---------------------------------- + Data : specific_humidity(latitude(5), longitude(8)) 1 + Cell methods : area: mean + Dimension coords: latitude(5) = [-75.0, ..., 75.0] degrees_north + : longitude(8) = [22.5, ..., 337.5] degrees_east + : time(1) = [2019-01-01 00:00:00] + >>> f.del_construct('time') + <{{repr}}DimensionCoordinate: time(1) days since 2018-12-01 > + >>> f.del_construct('time') + Traceback (most recent call last): + ... + ValueError: Can't find unique construct to remove + >>> f.del_construct('time', default='No time') + 'No time' + >>> f.del_construct('dimensioncoordinate1') + <{{repr}}DimensionCoordinate: longitude(8) degrees_east> + >>> print(f) + Field: specific_humidity (ncvar%q) + ---------------------------------- + Data : specific_humidity(latitude(5), ncdim%lon(8)) 1 + Cell methods : area: mean + Dimension coords: latitude(5) = [-75.0, ..., 75.0] degrees_north + + """ + # Need to re-define to overload this method since cfdm doesn't + # have the concept of cyclic axes, so have to update the + # register of cyclic axes when we delete a construct in cf. + + # Get the relevant key first because it will be lost upon deletion. + key = self.construct_key(*identity, default=None, **filter_kwargs) + # Copy since should never change value of _cyclic attribute in-place + cyclic_axes = self._cyclic.copy() + + deld_construct = super().del_construct( + *identity, default=default, **filter_kwargs) + + # If the construct deleted was a cyclic axes, remove it from the set + # of stored cyclic axes, to update that appropriately. Do this + # afterwards because the deletion might not be successful and don't + # want to update the cyclic() set unless we know the deletion occurred. + if key in cyclic_axes: + # The below is to test that the construct was successfully deleted + if isinstance(default, Exception) or ( + not isinstance(default, Exception) + and deld_construct != default + ): + cyclic_axes.remove(key) + self._cyclic = cyclic_axes + logger.info( + "Deleted a construct that corresponds to a cyclic axis " + f"({key}), so it was removed from the cyclic() axes set." + ) + def set_coordinate_reference( self, coordinate_reference, key=None, parent=None, strict=True ): From 555112176c3e3befbe2513b29bd7ecdf4044f8de Mon Sep 17 00:00:00 2001 From: "Sadie L. Bartholomew" Date: Mon, 22 Apr 2024 00:16:56 +0100 Subject: [PATCH 02/12] Add minimal unit test for Field.del_construct --- cf/test/test_Field.py | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/cf/test/test_Field.py b/cf/test/test_Field.py index 13b36bf426..6f64538b93 100644 --- a/cf/test/test_Field.py +++ b/cf/test/test_Field.py @@ -2572,6 +2572,30 @@ def test_Field_set_construct_conform(self): cm2 = f.cell_method("method:maximum") self.assertEqual(cm2.get_axes(), ("T",)) + def test_Field_del_construct(self): + """Test the `del_construct` Field method.""" + # Test a field without cyclic axes. These are equivalent tests to those + # in the cfdm test suite, to check the behaviour is the same in cf. + f = self.f1.copy() + print(f.cyclic()) + + self.assertIsInstance( + f.del_construct("auxiliarycoordinate1"), cf.AuxiliaryCoordinate + ) + + with self.assertRaises(ValueError): + f.del_construct("auxiliarycoordinate1") + + self.assertIsNone( + f.del_construct("auxiliarycoordinate1", default=None) + ) + + self.assertIsInstance( + f.del_construct("measure:area"), cf.CellMeasure + ) + + # TODO test cyclic nature + def test_Field_persist(self): """Test the `persist` Field method.""" f = cf.example_field(0) From c7e07cd1ba5fffc54525f670547af0f1532d0b56 Mon Sep 17 00:00:00 2001 From: "Sadie L. Bartholomew" Date: Mon, 22 Apr 2024 00:20:16 +0100 Subject: [PATCH 03/12] Update Field.del_construct unit test to check cyclic() tidy --- cf/test/test_Field.py | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/cf/test/test_Field.py b/cf/test/test_Field.py index 6f64538b93..53266dfc40 100644 --- a/cf/test/test_Field.py +++ b/cf/test/test_Field.py @@ -2594,7 +2594,19 @@ def test_Field_del_construct(self): f.del_construct("measure:area"), cf.CellMeasure ) - # TODO test cyclic nature + # Test a field with cyclic axes, to ensure the cyclic() set is + # updated accordingly if a cyclic axes is the one removed. + g = cf.example_field(2) # this has a cyclic axes 'domainaxis2' + # To delete a cyclic axes, must first delete this dimension coordinate + # because 'domainaxis2' spans it. + self.assertIsInstance( + g.del_construct("dimensioncoordinate2"), cf.DimensionCoordinate + ) + self.assertEqual(g.cyclic(), set(("domainaxis2",))) + self.assertIsInstance( + g.del_construct("domainaxis2"), cf.DomainAxis + ) + self.assertEqual(g.cyclic(), set()) def test_Field_persist(self): """Test the `persist` Field method.""" From c7b23e687023c5a011bf7ee2e2c298c7c2a0c0dc Mon Sep 17 00:00:00 2001 From: "Sadie L. Bartholomew" Date: Mon, 22 Apr 2024 00:28:09 +0100 Subject: [PATCH 04/12] Fix del_construct by returning del'd construct as in cfdm --- cf/mixin/fielddomain.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cf/mixin/fielddomain.py b/cf/mixin/fielddomain.py index c85a5379a9..0b9b9e883d 100644 --- a/cf/mixin/fielddomain.py +++ b/cf/mixin/fielddomain.py @@ -2509,6 +2509,8 @@ def del_construct(self, *identity, default=ValueError(), **filter_kwargs): f"({key}), so it was removed from the cyclic() axes set." ) + return deld_construct + def set_coordinate_reference( self, coordinate_reference, key=None, parent=None, strict=True ): From c7d4dccd8ac1d41c7c6a50286560ee048f2efe2f Mon Sep 17 00:00:00 2001 From: "Sadie L. Bartholomew" Date: Mon, 22 Apr 2024 00:38:09 +0100 Subject: [PATCH 05/12] Add Changelog entry for PR to fix Issue 758 --- Changelog.rst | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Changelog.rst b/Changelog.rst index e698df4f48..546aa97ecf 100644 --- a/Changelog.rst +++ b/Changelog.rst @@ -27,6 +27,9 @@ version 3.16.2 * Fix bug in `cf.aggregate` that sometimes put a null transpose operation into the Dask graph when one was not needed (https://github.com/NCAS-CMS/cf-python/issues/754) +* Fix bug whereby `Field.cyclic` is not updated after a + `Field.del_construct` operation + (https://github.com/NCAS-CMS/cf-python/issues/758) ---- From 0f6d0bff4382b2a33badfd9f78d59f9caf158c1f Mon Sep 17 00:00:00 2001 From: "Sadie L. Bartholomew" Date: Mon, 22 Apr 2024 10:15:51 +0100 Subject: [PATCH 06/12] Add minimal unit test for Domain.del_construct --- cf/test/test_Domain.py | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/cf/test/test_Domain.py b/cf/test/test_Domain.py index b548c553a0..3fe49342df 100644 --- a/cf/test/test_Domain.py +++ b/cf/test/test_Domain.py @@ -391,6 +391,42 @@ def test_Domain_create_regular(self): np.allclose(latitude_specific.array - y_points_specific, 0) ) + def test_Domain_del_construct(self): + """Test the `del_construct` Domain method.""" + # Test a field without cyclic axes. These are equivalent tests to those + # in the cfdm test suite, to check the behaviour is the same in cf. + d = self.d.copy() + + self.assertIsInstance( + d.del_construct("dimensioncoordinate1"), cf.DimensionCoordinate + ) + self.assertIsInstance( + d.del_construct("auxiliarycoordinate1"), cf.AuxiliaryCoordinate + ) + with self.assertRaises(ValueError): + d.del_construct("auxiliarycoordinate1") + + self.assertIsNone( + d.del_construct("auxiliarycoordinate1", default=None) + ) + + self.assertIsInstance( + d.del_construct("measure:area"), cf.CellMeasure + ) + + # Test a field with cyclic axes, to ensure the cyclic() set is + # updated accordingly if a cyclic axes is the one removed. + e = cf.example_field(2).domain # this has a cyclic axes 'domainaxis2' + # To delete a cyclic axes, must first delete this dimension coordinate + # because 'domainaxis2' spans it. + self.assertIsInstance( + e.del_construct("dimensioncoordinate2"), cf.DimensionCoordinate + ) + self.assertEqual(e.cyclic(), set(("domainaxis2",))) + self.assertIsInstance( + e.del_construct("domainaxis2"), cf.DomainAxis + ) + self.assertEqual(e.cyclic(), set()) if __name__ == "__main__": print("Run date:", datetime.datetime.now()) From 1d77686ff0944cfe4f85f7b5c75a12dbbbbbef2c Mon Sep 17 00:00:00 2001 From: "Sadie L. Bartholomew" Date: Mon, 22 Apr 2024 10:55:05 +0100 Subject: [PATCH 07/12] Tidy up dev. elements and version tags for PR --- cf/mixin/fielddomain.py | 4 ++-- cf/test/test_Domain.py | 6 +++--- cf/test/test_Field.py | 1 - 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/cf/mixin/fielddomain.py b/cf/mixin/fielddomain.py index 0b9b9e883d..b46894d8bb 100644 --- a/cf/mixin/fielddomain.py +++ b/cf/mixin/fielddomain.py @@ -2418,7 +2418,7 @@ def del_construct(self, *identity, default=ValueError(), **filter_kwargs): removed even if it is referenced by coordinate reference construct. - .. versionadded:: 3.16.2 + .. versionadded:: NEXTVERSION .. seealso:: `get_construct`, `constructs`, `has_construct`, `set_construct` @@ -2445,7 +2445,7 @@ def del_construct(self, *identity, default=ValueError(), **filter_kwargs): {{filter_kwargs: optional}} - .. versionadded:: (cfdm) 1.8.9.0 + .. versionadded:: NEXTVERSION :Returns: diff --git a/cf/test/test_Domain.py b/cf/test/test_Domain.py index 3fe49342df..5cd484c6c0 100644 --- a/cf/test/test_Domain.py +++ b/cf/test/test_Domain.py @@ -393,8 +393,8 @@ def test_Domain_create_regular(self): def test_Domain_del_construct(self): """Test the `del_construct` Domain method.""" - # Test a field without cyclic axes. These are equivalent tests to those - # in the cfdm test suite, to check the behaviour is the same in cf. + # Test a domain without cyclic axes. These are equivalent tests to + # those in the cfdm test suite, to check behaviour is the same in cf. d = self.d.copy() self.assertIsInstance( @@ -414,7 +414,7 @@ def test_Domain_del_construct(self): d.del_construct("measure:area"), cf.CellMeasure ) - # Test a field with cyclic axes, to ensure the cyclic() set is + # Test a domain with cyclic axes, to ensure the cyclic() set is # updated accordingly if a cyclic axes is the one removed. e = cf.example_field(2).domain # this has a cyclic axes 'domainaxis2' # To delete a cyclic axes, must first delete this dimension coordinate diff --git a/cf/test/test_Field.py b/cf/test/test_Field.py index 53266dfc40..2cd41a0d0a 100644 --- a/cf/test/test_Field.py +++ b/cf/test/test_Field.py @@ -2577,7 +2577,6 @@ def test_Field_del_construct(self): # Test a field without cyclic axes. These are equivalent tests to those # in the cfdm test suite, to check the behaviour is the same in cf. f = self.f1.copy() - print(f.cyclic()) self.assertIsInstance( f.del_construct("auxiliarycoordinate1"), cf.AuxiliaryCoordinate From 3d1507c0bdd2e9c78321d84b860fcfdbdd2fc38c Mon Sep 17 00:00:00 2001 From: "Sadie L. Bartholomew" Date: Mon, 22 Apr 2024 11:11:28 +0100 Subject: [PATCH 08/12] Add note to test_Domain RE influence of separate bug --- cf/test/test_Domain.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/cf/test/test_Domain.py b/cf/test/test_Domain.py index 5cd484c6c0..24c6850464 100644 --- a/cf/test/test_Domain.py +++ b/cf/test/test_Domain.py @@ -414,6 +414,10 @@ def test_Domain_del_construct(self): d.del_construct("measure:area"), cf.CellMeasure ) + # NOTE: this test will fail presently because of a bug which means + # that Field.domain doesn't inherit the cyclic() axes of the + # corresponding Field (see Issue #762) which will be fixed shortly. + # # Test a domain with cyclic axes, to ensure the cyclic() set is # updated accordingly if a cyclic axes is the one removed. e = cf.example_field(2).domain # this has a cyclic axes 'domainaxis2' From 3bc39ed9c36178952c2f14a2dcea0fcbb19393b8 Mon Sep 17 00:00:00 2001 From: "Sadie L. Bartholomew" Date: Mon, 22 Apr 2024 11:30:47 +0100 Subject: [PATCH 09/12] Apply black to lint updated files for PR --- cf/mixin/fielddomain.py | 9 +++++---- cf/test/test_Domain.py | 9 +++------ cf/test/test_Field.py | 8 ++------ 3 files changed, 10 insertions(+), 16 deletions(-) diff --git a/cf/mixin/fielddomain.py b/cf/mixin/fielddomain.py index b46894d8bb..6ea4260ff9 100644 --- a/cf/mixin/fielddomain.py +++ b/cf/mixin/fielddomain.py @@ -2490,7 +2490,8 @@ def del_construct(self, *identity, default=ValueError(), **filter_kwargs): cyclic_axes = self._cyclic.copy() deld_construct = super().del_construct( - *identity, default=default, **filter_kwargs) + *identity, default=default, **filter_kwargs + ) # If the construct deleted was a cyclic axes, remove it from the set # of stored cyclic axes, to update that appropriately. Do this @@ -2499,13 +2500,13 @@ def del_construct(self, *identity, default=ValueError(), **filter_kwargs): if key in cyclic_axes: # The below is to test that the construct was successfully deleted if isinstance(default, Exception) or ( - not isinstance(default, Exception) - and deld_construct != default + not isinstance(default, Exception) + and deld_construct != default ): cyclic_axes.remove(key) self._cyclic = cyclic_axes logger.info( - "Deleted a construct that corresponds to a cyclic axis " + "Deleted a construct that corresponds to a cyclic axis " f"({key}), so it was removed from the cyclic() axes set." ) diff --git a/cf/test/test_Domain.py b/cf/test/test_Domain.py index 24c6850464..9e5566eab0 100644 --- a/cf/test/test_Domain.py +++ b/cf/test/test_Domain.py @@ -410,9 +410,7 @@ def test_Domain_del_construct(self): d.del_construct("auxiliarycoordinate1", default=None) ) - self.assertIsInstance( - d.del_construct("measure:area"), cf.CellMeasure - ) + self.assertIsInstance(d.del_construct("measure:area"), cf.CellMeasure) # NOTE: this test will fail presently because of a bug which means # that Field.domain doesn't inherit the cyclic() axes of the @@ -427,11 +425,10 @@ def test_Domain_del_construct(self): e.del_construct("dimensioncoordinate2"), cf.DimensionCoordinate ) self.assertEqual(e.cyclic(), set(("domainaxis2",))) - self.assertIsInstance( - e.del_construct("domainaxis2"), cf.DomainAxis - ) + self.assertIsInstance(e.del_construct("domainaxis2"), cf.DomainAxis) self.assertEqual(e.cyclic(), set()) + if __name__ == "__main__": print("Run date:", datetime.datetime.now()) cf.environment() diff --git a/cf/test/test_Field.py b/cf/test/test_Field.py index 2cd41a0d0a..4a7a46db6b 100644 --- a/cf/test/test_Field.py +++ b/cf/test/test_Field.py @@ -2589,9 +2589,7 @@ def test_Field_del_construct(self): f.del_construct("auxiliarycoordinate1", default=None) ) - self.assertIsInstance( - f.del_construct("measure:area"), cf.CellMeasure - ) + self.assertIsInstance(f.del_construct("measure:area"), cf.CellMeasure) # Test a field with cyclic axes, to ensure the cyclic() set is # updated accordingly if a cyclic axes is the one removed. @@ -2602,9 +2600,7 @@ def test_Field_del_construct(self): g.del_construct("dimensioncoordinate2"), cf.DimensionCoordinate ) self.assertEqual(g.cyclic(), set(("domainaxis2",))) - self.assertIsInstance( - g.del_construct("domainaxis2"), cf.DomainAxis - ) + self.assertIsInstance(g.del_construct("domainaxis2"), cf.DomainAxis) self.assertEqual(g.cyclic(), set()) def test_Field_persist(self): From e3a6600ff3e3975e614f2ba166e1df85969c9408 Mon Sep 17 00:00:00 2001 From: "Sadie L. Bartholomew" Date: Tue, 23 Apr 2024 11:48:30 +0100 Subject: [PATCH 10/12] Update cf/mixin/fielddomain.py to remove log call for performance Co-authored-by: David Hassell --- cf/mixin/fielddomain.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/cf/mixin/fielddomain.py b/cf/mixin/fielddomain.py index 6ea4260ff9..6bb489efb7 100644 --- a/cf/mixin/fielddomain.py +++ b/cf/mixin/fielddomain.py @@ -2505,10 +2505,6 @@ def del_construct(self, *identity, default=ValueError(), **filter_kwargs): ): cyclic_axes.remove(key) self._cyclic = cyclic_axes - logger.info( - "Deleted a construct that corresponds to a cyclic axis " - f"({key}), so it was removed from the cyclic() axes set." - ) return deld_construct From b988c043dfeda7164e1c2f0049f41f853e0426b6 Mon Sep 17 00:00:00 2001 From: "Sadie L. Bartholomew" Date: Tue, 23 Apr 2024 11:49:31 +0100 Subject: [PATCH 11/12] Update cf/mixin/fielddomain.py to improve deletion status check Co-authored-by: David Hassell --- cf/mixin/fielddomain.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/cf/mixin/fielddomain.py b/cf/mixin/fielddomain.py index 6bb489efb7..73d1d17ed2 100644 --- a/cf/mixin/fielddomain.py +++ b/cf/mixin/fielddomain.py @@ -2490,8 +2490,13 @@ def del_construct(self, *identity, default=ValueError(), **filter_kwargs): cyclic_axes = self._cyclic.copy() deld_construct = super().del_construct( - *identity, default=default, **filter_kwargs + *identity, default=None, **filter_kwargs ) + if deld_construct is None: + if default is None: + return + + return self._default(default, "Can't find unique construct to remove") # If the construct deleted was a cyclic axes, remove it from the set # of stored cyclic axes, to update that appropriately. Do this From 4f271e59af2275d1bbc72686d0bc259641a0d41a Mon Sep 17 00:00:00 2001 From: "Sadie L. Bartholomew" Date: Tue, 23 Apr 2024 12:16:40 +0100 Subject: [PATCH 12/12] Update mixin.fielddomain del_construct for DH feedback --- cf/mixin/fielddomain.py | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/cf/mixin/fielddomain.py b/cf/mixin/fielddomain.py index 73d1d17ed2..3e1ea3ae83 100644 --- a/cf/mixin/fielddomain.py +++ b/cf/mixin/fielddomain.py @@ -2484,10 +2484,9 @@ def del_construct(self, *identity, default=ValueError(), **filter_kwargs): # have the concept of cyclic axes, so have to update the # register of cyclic axes when we delete a construct in cf. - # Get the relevant key first because it will be lost upon deletion. + # Get the relevant key first because it will be lost upon deletion key = self.construct_key(*identity, default=None, **filter_kwargs) - # Copy since should never change value of _cyclic attribute in-place - cyclic_axes = self._cyclic.copy() + cyclic_axes = self._cyclic deld_construct = super().del_construct( *identity, default=None, **filter_kwargs @@ -2495,21 +2494,20 @@ def del_construct(self, *identity, default=ValueError(), **filter_kwargs): if deld_construct is None: if default is None: return - - return self._default(default, "Can't find unique construct to remove") + + return self._default( + default, "Can't find unique construct to remove" + ) # If the construct deleted was a cyclic axes, remove it from the set - # of stored cyclic axes, to update that appropriately. Do this - # afterwards because the deletion might not be successful and don't - # want to update the cyclic() set unless we know the deletion occurred. + # of stored cyclic axes, to sync that. This is safe now, since given + # the block above we can be sure the deletion was successful. if key in cyclic_axes: - # The below is to test that the construct was successfully deleted - if isinstance(default, Exception) or ( - not isinstance(default, Exception) - and deld_construct != default - ): - cyclic_axes.remove(key) - self._cyclic = cyclic_axes + # Never change value of _cyclic attribute in-place. Only copy now + # when the copy is known to be required. + cyclic_axes = cyclic_axes.copy() + cyclic_axes.remove(key) + self._cyclic = cyclic_axes return deld_construct