search

NCAS-CMS / cf-python

A CF-compliant Earth Science data analysis library
http://ncas-cms.github.io/cf-python
MIT License
119 stars 19 forks source link

Allow a halo to be added by `indices` and `subspace` #760

Closed davidhassell closed 4 months ago

davidhassell commented 5 months ago

Fixes #759

Notes

The removed .html files were effectively empty, a bug resulting from methods being built as attributes, and vice versa.

davidhassell commented 4 months ago

Converting to draft whilst I fix some problems :)

davidhassell commented 4 months ago

Ready for review :)

davidhassell commented 4 months ago

All good for review - just added a couple of lines that will fix some of Bryan's performance issues, and are in the same area of code as touched by this PR.

davidhassell commented 4 months ago

Hi Sadie,

It seems like the subspace API is as it was before, so doesn't include the capability to specify the halo, but I thought the intention was to allow that for both methods?

subspace does indeed have the new API, but I forgot to update its docstring. Sorry!

davidhassell commented 4 months ago

... but I forgot to update its docstring ...

The reason for this is that the docstring you generally see is in cf.subspacefield.py, rather than cf/field.py. I had only updated the latter. This is pointless complexity [*] which we should get rid of for v4.0.0.

[*] i.e. the ability to square-bracket index the subspace attribute (edit)

davidhassell commented 4 months ago

Ideally we can add test coverage to include the use of both mode and halo settings as positional arguments

Done: 0a8b8a067f066e771f526b85ce8fabdfef8fc955

davidhassell commented 4 months ago

Also added an API test to subspace: 31ec154bd7a2e518782c9018ea2cafe06c5fad41