Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

style(tracingfuncs): Numpydoc validation, PEP8 & tidying #948

Conversation

ns-rse
Copy link
Collaborator

@ns-rse ns-rse commented Oct 14, 2024

Resolves #920

This commit is a squash of seven individual commits that address Numpydoc validation

  • style(tracingfuncs): GL01/GL02 Numpydoc validation
  • style(tracingfuncs): SS06 Numpydoc validation
  • style(tracingfuncs): GL08 Numpydoc validation
  • style(tracingfuncs): PR01/RT01 Numpydoc validation

Removes unused functions/methods...

  • fix(tracingfuncs): Removes circularTrace_old
  • fix(tracingfuncs): Remove unused getLocalPixelsBinary

Renames methods to adhere to PEP8

  • style(tracingfuncs): PEP8 renaming camel-case method to snake-case

Questions

check_vectors_candidate_points - part 1

This function (nee checkVectorsCandidatePoints) takes the parameters x and y but these are never used directly,
rather x and y are obtained anew in each iteration of this loop...

        for x_n, y_n in candidate_points:
            x = x_n - x_ref_2
            y = y_n - y_ref_2

            theta = math.atan2(x, y)

            x_y_theta.append([x_n, y_n, abs(theta - ref_theta)])

We should be able to just drop x and y as parameters to this method but I wanted to check as there aren't any tests
for these methods/functions.

check_vectors_candidate_points - part 2

I also noticed that in some places check_vectors_candidate_points (nee checkVectorsCandidatePoints)
was commented out (lines 83 and 187) in favour of using find_next_best_point (nee findNextBestPoint) but
check_vectors_candidate_points is still used in some places.

  • Is this deliberate?
  • Can or should find_next_best_point replace all calls made to check_vectors_candidate_points?

This commit is squash of seven individual commits that address Numpydoc validation

- style(tracingfuncs): GL01/GL02 Numpydoc validation
- style(tracingfuncs): SS06 Numpydoc validation
- style(tracingfuncs): GL08 Numpydoc validation
- style(tracingfuncs): PR01/RT01 Numpydoc validation

Removes unused functions/methods...

- fix(tracingfuncs): Removes circularTrace_old
- fix(tracingfuncs): Remove unused getLocalPixelsBinary

Renames methods to adhere to PEP8

- style(tracingfuncs): PEP8 renaming camel-case method to snake-case
@MaxGamill-Sheffield
Copy link
Collaborator

@ns-rse I have a rough idea what this piece of legacy code does but not how different things could be interchanged. I'd say leave this for now and get this adhering to the standards. And possibly make a new issue to swap these out and see if any tests are affected?

Copy link
Collaborator

@MaxGamill-Sheffield MaxGamill-Sheffield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just if you want to refactor the legacy code or make a new issue?

MaxGamill-Sheffield and others added 13 commits October 15, 2024 15:40
Co-authored-by: Neil Shephard <[email protected]>
Co-authored-by: Neil Shephard <[email protected]>
I've also...

- Added place holders for missing disordered_tracing tests.
- Removed hard coding of variable renaming which replaced `-` with `_` so its generic.
Means we can remove `# pylint: disable=unnecessary-pass` from these files and we get a reminder of what tests still need
writing.
As noted in the [PR #948](#948 (comment)) the `x` and `y` parameters to
`check_vectors_candidate_points()` were redundant and not being used as the function looped over points from the
`candidate_points` parameters instead. They have therefore been removed.
@ns-rse
Copy link
Collaborator Author

ns-rse commented Oct 15, 2024

Looks good, just if you want to refactor the legacy code or make a new issue?

I've gone with somewhere in-between and addressed Part 1 as removing the unused parameters had no impact on the tests completing successfully and kicked the can done the road for Part 2 and written it up in #953

@ns-rse ns-rse merged commit 222e7a8 into maxgamill-sheffield/800-better-tracing Oct 15, 2024
10 checks passed
@ns-rse ns-rse deleted the ns-rse/920-tracingfuncs-numpydoc-validation branch October 15, 2024 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants