ordered errors for all but cell method constructs

[sadielbartholomew]

On the latest master the ordered method on a Constructs class appears to successfully return only for cell methods, returning a ValueError for a Construct object containing any other valid and same-type constructs, e.g:

>>> import cfdm
>>> f = cfdm.example_field(6)
>>> c = f.constructs()
>>> c
<Constructs: auxiliary_coordinate(4), coordinate_reference(1), dimension_coordinate(1), domain_axis(2)>
>>> a = c.filter_by_type('auxiliary_coordinate')
>>> a
<Constructs: auxiliary_coordinate(4)>
>>> a.ordered()
{'cell_method'} {'auxiliary_coordinate'}
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/sadie/cfdm/cfdm/core/constructs.py", line 1240, in ordered
    raise ValueError(
ValueError: Can't order un-orderable construct type: <Constructs: auxiliary_coordinate(4)>
>>> b.ordered()
{'cell_method'} {'domain_axis'}
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/sadie/cfdm/cfdm/core/constructs.py", line 1240, in ordered
    raise ValueError(
ValueError: Can't order un-orderable construct type: <Constructs: domain_axis(2)>

where the output lines before the tracebacks are emerging because I added the following print call into the method for debugging:

diff --git a/cfdm/core/constructs.py b/cfdm/core/constructs.py
index 575ffd342..f09490f8d 100644
--- a/cfdm/core/constructs.py
+++ b/cfdm/core/constructs.py
@@ -1237,6 +1235,7 @@ class Constructs(abstract.Container):
                 "Can't order multiple construct types: {!r}".format(self)
             )
 
+        print(self._ordered_constructs, set(self._constructs))
         if self._ordered_constructs != set(self._constructs):
             raise ValueError(
                 "Can't order un-orderable construct type: {!r}".format(self)

and the first print item is always {'cell_method'}, demonstrating that only cell method constructs seem to be able to be ordered by the method. But that doesn't seem right, especially as this is a generic construct method as implied by the docstring?

It appears that, once same-type constructs are input, it reaches final logic that only deals with cell method constructs because only they get added to the _ordered_constructs instance attribute dict in the line:

cfdm/cfdm/core/constructs.py

Line 248 in e908dc8

self._ordered_constructs.add("cell_method")

Note that this behaviour gets passed downstream and also manifests in cf-python (the initial code snippet above has the same results when cf is substituted for cfdm).

@davidhassell it would be useful to hear you thoughts: should other types of constructs be processed and if they should, is it the case, as I suspect and as implied by this line in the docstring:

cfdm/cfdm/core/constructs.py

Lines 1205 to 1206 in e908dc8

	For cell method constructs, the predetermined order is that in
	which they where added.

that cell methods should be treated as a special case such that some logic is missing to handle the ordering of all other constructs? Thanks.

[davidhassell]

Hi @sadielbartholomew, the short answer is that in the current CF data model, cell method constructs must be orderable but there is no defined ordering for any other type of construct (such as dimension coordinate constructs).

Would this change to the docstring work?

        For cell method constructs, the predetermined order is that in
        which they where added. There is no predetermined ordering for all other
        construct types, and a exception is raised if any non-cell method constructs
        are present.

[davidhassell]

Just had a look around the code a bit more .... (e.g.

cfdm/cfdm/core/constructs.py

Lines 256 to 257 in e908dc8

	for x in self._ordered_constructs:
	self._constructs[x] = OrderedDict()

)

I'm reminded that some of this logic stems from the fact that pre Python3.7 we needed to use OrderedDict for cell methods, but that is not necessary for 3.7 onwards. I think we are close to dropping 3.6 support (meaning we should do this!), as it is no longer supported by numpy 1.20. In this case, we can simplify (and speed up!) the code by getting rid of the "ordered" functionality completely (including the Constructs.ordered method) - we just say that constructs, of any flavour, are returned in the order in which they were set. This order happens to be arbitrary for all constructs except cell methods, for which this order determines the sequence in which the cell methods were applied to the data.

How does that sound?

[sadielbartholomew]

Thanks for the clarification @davidhassell. That makes sense, though as it is currently I don't think that context is clear (I certainly got confused as you can see!).

Would this change to the docstring work?

That makes it much clearer, thanks. Would you like to make that change or shall I (I guess we can't drop 3.6 support immediately for the next release)?

In this case, we can simplify (and speed up!) the code by getting rid of the "ordered" functionality completely (including the Constructs.ordered method) - we just say that constructs, of any flavour, are returned in the order in which they were set.

Brilliant, let's do that. Simple and means we don't need to treat cell methods as the one orderable special case.

[davidhassell]

Hi @sadielbartholomew - great, sounds like we have a plan, then. I'm happy to make the suggested change to the docstring for now, and we'll refactor the Constructs object for a future (not 1.8.9.0) release.

[sadielbartholomew]

Great, thanks @davidhassell.