Should cubelists contain objects that are not cubes?

[rcomer]

Due to a somewhat noddy coding error, I learned that it is possible to add None to a cubelist via the append method. This leads to subsequent code breaking with a confusing error message.

I think it would be straightforward to add a check into CubeList.append so that it only takes a cube. I'd be happy to have a go at a PR to do this, but I noticed the comment in CubeList.__new__, which suggests that this was thought about but not done. Is there a reason why restricting the append method would be a bad idea?

[pelson]

I suspect the only reason for that is related to performance concerns. I don't know if that is a valid concern or not - we'd certainly have to instrument any change we make on this part of the code to ensure we aren't introducing any major bottlenecks. Otherwise, I'm certainly in favour of ensuring a cubelist always contains cubes.

[ajdawson]

What operation were you performing on the CubeList and what was the confusing error message?

[rcomer]

The operation was along the lines of

[cube] = cubelist.extract(name)

the error was

AttributeError: 'NoneType' object has no attribute 'ndim'

if you try to print a cubelist with None in it, you get

AttributeError: 'NoneType' object has no attribute 'summary'

[marqh]

If we allow a None, we shouldn't raise an exception when trying to print a None; in my opinion.

I'm also in favour of enforcing that the contents of CubeLists are Cubes

[rcomer]

In the interest of putting some numbers on this, I have:

import iris

def no_check(cubelist, cube):
    cubelist.append(cube)
    
def check(cubelist, cube):
    if isinstance(cube, iris.cube.Cube):
        cubelist.append(cube)
    else:
        raise TypeError('Cubes only in a cubelist!')
        
        
mycube = iris.cube.Cube(1)
mycubelist = iris.cube.CubeList()

Then

$ python -m timeit -r 10 -s 'import cubelist_append' "cubelist_append.no_check(cubelist_append.mycubelist, cubelist_append.mycube)"
1000000 loops, best of 10: 0.405 usec per loop
$ python -m timeit -r 10 -s 'import cubelist_append' "cubelist_append.check(cubelist_append.mycubelist, cubelist_append.mycube)"
1000000 loops, best of 10: 0.648 usec per loop

[pp-mo]

also in favour of enforcing that the contents of CubeLists are Cubes

gets my 👍

[rcomer]

This week I made the same daft mistake in a different project, and this time I learned that if you try to save a cubelist with a None to NetCDF you get

AttributeError: 'NoneType' object has no attribute 'attributes'

I'll be the first to admit that #3238 has got out of hand(!) so I'm wondering if there is mileage in a new PR that only addresses append as it's probably the method that's most commonly used. Maybe also fix the __new__/__init__ issue discussed in #3238.

[rcomer]

Attempts were made (#3238 and #3510) but there doesn’t appear to be a simple solution to this right now.

I’ll leave this open as it’s possible someone might come up with a neat solution later. Otherwise it can stand as a warning to the unwary 😆

[rcomer]

I'm going to close this as I think the effort to address it will be disproportionate to the size of the problem. Please comment and/or re-open if you think otherwise!

[pp-mo]

Sorry to hear of the demise here, since you put your effort into it !

Another approach I have been wondering about is whether it was a bad idea to invent CubeList at all, and we could get rid of it.
#4414

[rcomer]

@pp-mo you're very welcome to resurrect it if you're keen 😆

I just thought I might as well preempt the stale bot.

[jamesp]

Would some type hints on CubeList functions (such as append) avoid the worst of this, without changing any functional code? It would at least warn you in some editors then if you might be inadvertently be appending None to a list?

[rcomer]

I don't know much about how type-hinting works but presumably we would need to explicitly define those functions? At the moment we get append, etc. for free. Part of the reason we doubted the wisdom of #3238 was that it introduced rather a lot of new code for the size of the problem.

[jamesp]

I wonder if this works?

from typing import List
class CubeList(List["Cube"]):
    """
    All the functionality of a standard :class:`list` with added "Cube"
    context.

    """
...

I've just tried it in my vscode which seems happy, running the tests now. Edit: tests pass

[rcomer]

Thanks @jamesp, I've just been playing around with this. I think this wouldn't help my original problem, which can be illustrated by

def my_cube():
    cube = iris.cube.Cube(1)
    # forgot to add return statement!

l = iris.cube.CubeList()
l.append(my_cube())

mypy doesn't seem to pick the above up as a problem.

But

@pp-mo mentioned another case where a user had a cubelist within a cubelist, presumably because they'd used e.g. iris.load instead of iris.load_cube. So if we also add type hints to iris.load, etc., it does pick that up.

fname = "/path/to/my/file.nc"
l.append(iris.load(fname))

[jamesp]

This is strange, it seems to be by design in mypy not to follow through the known explicit None that is returned from an untyped function with no return statement. There are many issues on mypy debating the pros and cons of this.
For the simplest example:

def double(x):
    y = x*2
    # whoops, forgot to return

double(10) + 7

running mypy on this it is perfectly happy. Pylance (which uses the PyRight type checker under the covers) is more strict about this and does mark it as an error in my editor (VSCode). If you were to annotate your my_cube function then mypy would pick it up, which I suppose makes sense - if you are explicitly running mypy over your code it will only check the typed parts of it. I'm not a PyCharm user but maybe someone who does could take a look at what the PyCharm linter makes of all this.

[jamesp]

The simplest example demonstrating the mypy (not) type checking behaviour. VSCode linter detects the error in all append calls.

from typing import List

def make_str():
    a = "my string"

class StrList(List[str]):
    pass

sl = StrList()

sl.append("hello")    # all good
sl.append(1)          # mypy catches this
sl.append(None)       # mypy catches this
sl.append(make_str()) # mypy does not catch this, despite static analysis
                      # knowing that make_str() returns None

[rcomer]

Thanks for checking that @jamesp. I did try with VSCode too this afternoon, but couldn’t figure out how to get it working. I am a type-hint n00b.

[rcomer]

This was done at #4767 🎉