Segmentation

[isaac-tfn]

Describe Changes

Task Checklist

• [ ] Have unittests been added (testing individual functions and their edge cases)
• [ ] Have integration tests been added (if applicable, to test modules of functionality)
• [ ] Updated RELEASE.md to reflect changes in PR
• [ ] Have new dependencies been added?
  • [ ] Have they been added to either requirements.txt or requirements_dev.txt.
  • [ ] Have they been added to setup.cfg

[isaac-tfn]

I think most of my comments don't need fixing now and could just be added as issues (I can add those if needed). The main things I've noticed are the following:

• Segment default values should be dict / list to avoid needing all the None checks everywhere
• Docstrings should use numpy format
• Some type annotations are missing, mostly return types but also Literal strings where relevant

I haven't looked at the tests but can if needed.

Definitely need to improve documentation and annotations. On the None type checks, @BenTaylor-TfN has told me it's bad practice to default to lists/dicts (mutable types) which is why often they default to None. It's possible I'm misunderstanding this comment or I've misunderstood Ben at some point!

[wsp-mbuckley]

I think most of my comments don't need fixing now and could just be added as issues (I can add those if needed). The main things I've noticed are the following:

• Segment default values should be dict / list to avoid needing all the None checks everywhere
• Docstrings should use numpy format
• Some type annotations are missing, mostly return types but also Literal strings where relevant

I haven't looked at the tests but can if needed.

Definitely need to improve documentation and annotations. On the None type checks, @BenTaylor-TfN has told me it's bad practice to default to lists/dicts (mutable types) which is why often they default to None. It's possible I'm misunderstanding this comment or I've misunderstood Ben at some point!

@BenTaylor-TfN is correct, but pydantic has a Field class which can have default factory functions which it will call to instantiate a new list each time the class is instantiated it works similar to having functions default to None then inside the function doing

if variable is None:
    variable = []

In the comments by the code I've put an example of how to use this pydantic class. The other option is to use pydantic's validator with always=True to set to an empty list if the value is None but this is less neat.

The reason I think they shouldn't be allowed to be None is that there are lots of None checks which clutter up other pieces of code and they could be removed if the list / dict weren't allowed to be None, it wouldn't need any empty list / dict checks either because iterating through the empty collection just wouldn't have any iterations.

Can setup a call to go through this and other comments if you like?

[isaac-tfn]

Can setup a call to go through this and other comments if you like?

Sounds good thanks - doesn't look like I have many calls Thursday Friday so slot something in.

[wsp-mbuckley]

I've added a few issues which should cover all the comments I've made previously #11, #12, #13, #14, #15

[codecov-commenter]

Welcome to Codecov :tada:

Once merged to your default branch, Codecov will compare your coverage reports and display the results in this comment.

Thanks for integrating Codecov - We've got you covered :open_umbrella:

[isaac-tfn]

@wsp-mbuckley Still some linting stuff - a lot of pydocstyle, some mypy, pylint is all passing. I'm happy to pick it up after Christmas if you don't have time, should take a day or so just don't have time now.

[wsp-mbuckley]

@wsp-mbuckley Still some linting stuff - a lot of pydocstyle, some mypy, pylint is all passing. I'm happy to pick it up after Christmas if you don't have time, should take a day or so just don't have time now.

@isaac-tfn thanks for that, I've got some viz stuff to do next week but if a get time I'll try a get some of this done too so its ready for merging in the new year