Container copy method implies deep copy behaviour

[ncas-cms]

Container copy method implies deep copy behaviour

[davidhassell]

as I recall you suggested a key in _custom might be good.

I did indeed! Seemed like a good idea at the time ...

If we say now that the rule for _custom is that it is only for external subclasses, perhaps that will clarify things for us and others?

_inplace_enabled sounds good to me

[sadielbartholomew]

If we say now that the rule for _custom is that it is only for external subclasses, perhaps that will clarify things for us and others?

Sounds good to me, yes.

_inplace_enabled sounds good to me

Right-o, I'll do the conversion. Thanks.

[davidhassell]

See #117 for changes to the _custom docs

[sadielbartholomew]

@davidhassell I assume this is a bug and if so it would be good to discuss the best way to fix it. I have not investigated how far the shallowness of copy propagates throughout the codebase to inherited methods that do not override it, but this could be potentially a widespread issue? From looking at other failing doctests, this does occur downstream e.g. applies to the copy methods on auxiliarycoordinate, bounds and cellmeasure etc.

Once I have prepared all of the doctest examples and included them in the main test suite run (currently I just have a -d option to run them separately), we will have explicit tests against this, since the docstring to the _custom method runs essentially the same code I have in my minimal example above:

cfdm/cfdm/core/abstract/container.py

Lines 183 to 207 in 58d8fea

	def _custom(self):
	"""Customisable storage for additional attributes.
	.. versionadded:: (cfdm) 1.7.4
	**Examples:**
	>>> f = {{package}}.{{class}}()
	>>> f._custom
	{}
	>>> f._custom['feature'] = ['f']
	>>> g = f.copy()
	>>> g._custom['feature'][0] = 'g'
	>>> f._custom
	{'feature': ['f']}
	>>> g._custom
	{'feature': ['g']}
	>>> del g._custom['feature']
	>>> f._custom
	{'feature': ['f']}
	>>> g._custom
	{}
	"""
	return self._get_component("custom")

[davidhassell]

Hi Sadie - not a bug, rather a performance feature! and the reason why the _custom attribute has the underscore. Deep copying this dictionary could be a major performance hit for anything that uses it (like cf-python), which is unknown to cfdm. If the custom dictionary contains any mutables (not including any which are considered copy-on-write), then it is up to the the parent class to copy those explicitly - either in its own copy method, or else inside an if source is not None: clause in its __init__ method.

Currently, cfdm only uses _custom in decorators.py (https://github.com/NCAS-CMS/cfdm/blob/master/cfdm/decorators.py#L47-L53) - but I now think that we should not use it or this purpose - i.e. reserve _custom for applications that subclass cfdm. The decorator use could instead easily be satisfied with a normal underscore attribute.

So I think that the _custom attribute docstring is correct, but it clearly needs a lot more explanation on the consequences of it. The closest we get is https://github.com/NCAS-CMS/cfdm/blob/master/cfdm/core/abstract/container.py#L31-L32, but that's not much use to developers. This needs to be described much more explicitly, particularly for those subclassing cfdm.

Thanks, David

[sadielbartholomew]

Hi @davidhassell, thanks for the clarification. That's good news if it is expected behaviour and therefore not a bug.

So if I understand correctly, it seems I have misunderstood between the deep copying of the objects and the dictionary attributes attached to them? That sounds very plausible given the complexities of copying, apologies!

However there are numerous doctests essentially equivalent to the example I raise in the opening comment that are still failing because they are expecting f._custom to be {'feature': ['f']} rather than {'feature': ['g']}. But you seem to be implying that docstring is correct, but also that the behaviour is as expected. So I am left unsure as to how to handle that docstring. Am I missing something still?

Currently, cfdm only uses _custom in decorators.py (https://github.com/NCAS-CMS/cfdm/blob/master/cfdm/decorators.py#L47-L53) - but I now think that we should not use it or this purpose - i.e. reserve _custom for applications that subclass cfdm. The decorator use could instead easily be satisfied with a normal underscore attribute.

Fair point, that seems wise. Shall I make that change or would you prefer to? Is there a name you have in mind, otherwise perhaps _cfdm_custom, _cfdm_internals or something similar (that doesn't risk a nameclash and I would think better to have a more flexible name that, say, _cfdm_decorators in case we want to throw other things in that dict)?

[davidhassell]

Hi - OK! I've now read the _custom docstring a bit more clearly. I think it is wrong. I seem to vaguely remember that it was deep copied at some dim and distant past whilst cfdm was being developed. Seeing as we explicitly only shallow copy it, it should probably just say something like:

 """Customisable storage for additional attributes.

        .. versionadded:: (cfdm) 1.7.4

        **Examples:**

        >>> f = {{package}}.{{class}}()
        >>> f._custom
        {}
        >>> f._custom['feature'] = 1
        >>> g = f.copy()
        >>> g._custom
        {'feature':1}
        >>> del g._custom['feature']
        >>> g._custom
        {}
        >>> f._custom
        {'feature': 1}

        """

I'll think about some text to explain this in https://ncas-cms.github.io/cfdm/extensions.html

Fair point, that seems wise. Shall I make that change or would you prefer to? Is there a name you have in mind, otherwise perhaps _cfdm_custom, _cfdm_internals or something similar (that doesn't risk a nameclash and I would think better to have a more flexible name that, say, _cfdm_decorators in case we want to throw other things in that dict)?

Do such things (currently just _custom[INPLACE_ENABLED_PLACEHOLDER]) need to be in a dict at all? Couldn't we just assign them as attributes in any case (e.g. https://github.com/NCAS-CMS/cfdm/blob/master/cfdm/decorators.py#L58-L61)?

[sadielbartholomew]

Hi, thanks @davidhassell. I'll change the relevant docstrings accordingly.

Do such things (currently just _custom[INPLACE_ENABLED_PLACEHOLDER]) need to be in a dict at all? Couldn't we just assign them as attributes in any case (e.g. https://github.com/NCAS-CMS/cfdm/blob/master/cfdm/decorators.py#L58-L61)?

Ah, very good point, probably best use a simple attribute. I seem to remember last Jan/Feb when implementing the first decorators it was going to be simply INPLACE_ENABLED_PLACEHOLDER but as I recall you suggested a key in _custom might be good.

Maybe we can use a snappier attribute name than INPLACE_ENABLED_PLACEHOLDER and not capitalised, like _inplace_enabled or similar. Do you have a preference or suggestion or shall I pick something sensible?

[sadielbartholomew]

Ah, excellent, thanks. That was quick! Are you waiting on the CI results or would you like me to review (seems simple enough that it might not be necessary, but can do if you wish)?

I've realised the doctest result output made it seem like there were numerous methods having that particular docstring example, but in fact it is only one case that appears multiple times across the methods in the various modules by inheritance. So this one fix will reduce the number of doctest failures raised significantly :)