I noticed whilst developing the tests that the tests on a circular molecule with height_bias = 0.6 results in a strange skeleton. The ring of the circle is correct but there is a branch in the bottom right (search tests/tracing/test_skeletonize.py for TopoStats, circular, height_bias 0.6 and the array that is produced is above this).
This probably isn't what is expected but is worth giving some consideration to now because if such artifacts are produced on simple test cases its quite possible that we will get unwanted artifacts in more complex situations.
Additional Linting
Because I modified topostats/tracing/skeletonize.py it was linted as part of the pre-commit checks which include ruff, pylint and now numpydoc-validation, it had a lot of issues. I have therefore linted this file and...
Corrected all docstrings to pass validation. There were some that took ages to find and rectify. In the end I had to rewrite the docstrings from scratch for about 20 classes/methods as there was a hidden character that caused things to fail, quite possibly a hangover from when files might have been edited under Windows.
I've left notes where I couldn't understand what functions/methods were doing, these can be found by searching for ???.
Disabled a number of checks for now as most were in the other classes I've not started writing tests/reviewing yet (i.e. topostatsSkeletonize / pruneSkeleton / topostatsPrune / convPrune / heightPruning)
Some of the disabled checks (line numbers are rough but should get you close)
Ruff
topostats/tracing/skeletonize.py:597:34: B006 Do not use mutable data structures for argument defaults
topostats/tracing/skeletonize.py:775:9: C901 `_prune_by_length` is too complex (11 > 10)
Pylint
topostats/tracing/skeletonize.py:1:0: C0302: Too many lines in module (1399/1000) (too-many-lines)
topostats/tracing/skeletonize.py:17:0: R0903: Too few public methods (1/2) (too-few-public-methods)
topostats/tracing/skeletonize.py:187:0: R0902: Too many instance attributes (13/7) (too-many-instance-attributes)
topostats/tracing/skeletonize.py:595:4: W0102: Dangerous default value {} as argument (dangerous-default-value)
topostats/tracing/skeletonize.py:567:0: R0903: Too few public methods (1/2) (too-few-public-methods)
topostats/tracing/skeletonize.py:768:36: E1121: Too many positional arguments for method call (too-many-function-args)
topostats/tracing/skeletonize.py:773:4: R0914: Too many local variables (18/15) (too-many-locals)
topostats/tracing/skeletonize.py:685:0: R0903: Too few public methods (1/2) (too-few-public-methods)
topostats/tracing/skeletonize.py:958:36: E1121: Too many positional arguments for method call (too-many-function-args)
topostats/tracing/skeletonize.py:875:0: R0903: Too few public methods (1/2) (too-few-public-methods)
topostats/tracing/skeletonize.py:1179:4: R0914: Too many local variables (16/15) (too-many-locals)
topostats/tracing/skeletonize.py:1335:0: R0914: Too many local variables (19/15) (too-many-locals)
Some hopefully useful comments on how to resolve some of these...
topostatsSkeletonize class has too-many-instance-attributes this could be solved by converting self.p# to be a dictionary called self.points which would then have keys/values for each of the points.
Shouldn't pass dictionaries (dangerous-default-value) instead should use kwargs. From memory these can carry through functions without being unpacked, so the function/method that is called has `kwargsdefined and that is passed on as**kwargs` to the setting function right down to the final method that will use the values.
When PyLint reports too-many-locals it generally indicates that functionality shold be broken out into smaller functions.
More importantly it is considerably easier to address these during development/writing of code and not at the end in a big "clean-up". We have pre-commit hooks in place and should use them and address issues that are reported on every commit even if it is tempting to bypass them.
First step in adding tests to the
maxgamill-Sheffield/800-better-tracing.Tests
get_skeleton()function to work withgetSkeleton().tests/conftest.pytotests/tracing/conftest.py."topostats"method with different values ofheight_bias.getSkeletonclass/method, these are flagged as w0102dangerous-default-valueby Pylint and B006mutable-argument-defaultby Ruff.IMPORTANT
I noticed whilst developing the tests that the tests on a circular molecule with
height_bias = 0.6results in a strange skeleton. The ring of the circle is correct but there is a branch in the bottom right (searchtests/tracing/test_skeletonize.pyforTopoStats, circular, height_bias 0.6and the array that is produced is above this).This probably isn't what is expected but is worth giving some consideration to now because if such artifacts are produced on simple test cases its quite possible that we will get unwanted artifacts in more complex situations.
Additional Linting
Because I modified
topostats/tracing/skeletonize.pyit was linted as part of thepre-commitchecks which includeruff,pylintand nownumpydoc-validation, it had a lot of issues. I have therefore linted this file and...???.topostatsSkeletonize/pruneSkeleton/topostatsPrune/convPrune/heightPruning)Some of the disabled checks (line numbers are rough but should get you close)
Ruff
Pylint
Some hopefully useful comments on how to resolve some of these...
topostatsSkeletonizeclass hastoo-many-instance-attributesthis could be solved by convertingself.p#to be a dictionary calledself.pointswhich would then have keys/values for each of the points.dangerous-default-value) instead should use kwargs. From memory these can carry through functions without being unpacked, so the function/method that is called has `kwargsdefined and that is passed on as**kwargs` to the setting function right down to the final method that will use the values.too-many-localsit generally indicates that functionality shold be broken out into smaller functions.More importantly it is considerably easier to address these during development/writing of code and not at the end in a big "clean-up". We have
pre-commithooks in place and should use them and address issues that are reported on every commit even if it is tempting to bypass them.