Allow a halo to be added by indices and subspace

[davidhassell]

Fixes #759

Notes

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

[davidhassell]

Converting to draft whilst I fix some problems :)

[davidhassell]

Ready for review :)

[davidhassell]

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.

[sadielbartholomew]

Just finishing up on functionality testing, though so far that is all good. Here are my comments from considering the code itself. Mostly these are questions that occurred to me and typo fixes, with one strong preference to move to use an assert statement.

[sadielbartholomew]

Functionality-related comments now (plus a few more code eyeballing related). Overall, it appears to work well but I would advise for the mode API to be updated and consistent across both methods (I think this was the intention as per the original/corresponding Issue, unless I have misinterpreted it).

indices

The indices aspect seems to work fine, though I think it would be beneficial to add some new examples to the docstring to show usage of halo size setting as a positional argument, with and without also setting the mode component.

Ideally we can add test coverage to include the use of both mode and halo settings as positional arguments, since the new testing in test_Field doesn't seem to have any, unless I've missed something.

subspace

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? I certainly think both should include the mode and halo specification (now under config as per our discussions on here) and in the same manner, for consistency and since, for example with my VISION script, it would be nice to directly do:

model_field_sbb_indices = model_field.subspace(
    1,
    X=cf.wi(obs_X.minimum(), obs_X.maximum()),
    Y=cf.wi(obs_Y.minimum(), obs_Y.maximum()),
    Z=cf.wi(obs_Z.minimum(), obs_Z.maximum()),
)

instead of what I am doing for our spatial bounding box subspace using this branch, namely:

model_field_sbb_indices = model_field.indices(
    1,
    X=cf.wi(obs_X.minimum(), obs_X.maximum()),
    Y=cf.wi(obs_Y.minimum(), obs_Y.maximum()),
    Z=cf.wi(obs_Z.minimum(), obs_Z.maximum()),
)
model_field_sbb = model_field[model_field_sbb_indices]

If we do update the API for subspace in a similar way, testing should really be added for that as well, not just the indices side of things.

[davidhassell]

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]

... 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]

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

Done: 0a8b8a0

[davidhassell]

Also added an API test to subspace: 31ec154

[sadielbartholomew]

Thanks David for addressing my feedback so well. I think everything has been accounted for, and I've checked works and everything has been accordingly, except:

it would be beneficial to add some new examples to the docstring to show usage of halo size setting as a positional argument

for both the indices and subspace docstrings. The config setting with two positional arguments in particular is not yet shown in either case, and even the old lone mode string configuration seems to not be demonstrated for the indices case.

See also the pair of new comments in-line (sorry, I submitted one of two as a separate review before accidentally, so see above too) but overall, I am happy so please merge when you have considered those.