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

Linting and Formatting #19

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

CodingPenguin1
Copy link

Changes

Many changes were done to improve readability, including, but not limited to, the following:

  • Newlines added to separate logical blocks of code
  • Newlines removed to bring lines together into logical blocks of code
  • if x is True changed to if x
  • Docstring endings standardized to visibly separate code from documentation
  • Print statement quotes standardized to ' (was using " and """ in some places)
  • Added blanket excepts to empty except blocks
  • Single line functions broken into multiple lines for readability

Questions for Maintainers

Line 292 Try/Except

    try:
        x = globals()[method](hb_err, x0, **kwargs)

    except Exception as exception:
        x = x0  # np.full([x0.shape[0],x0.shape[1]],np.nan)
        amps = np.full([x0.shape[0], ], np.nan)
        phases = np.full([x0.shape[0], ], np.nan)
        e = hb_err(x)  # np.full([x0.shape[0],x0.shape[1]],np.nan)
        raise

Raising an exception terminates the code. Why is there code in the except block if it will do nothing? The raise statement should pass a string to be in the exception message. This provides no insight as to why the code failed.

Line 615 Try/Except

    try:
        X = globals()[method](hb_err, X0, **kwargs)
        e = hb_err(X)
        if mask_constant:
            X = np.hstack((np.zeros_like(X[:, 0]).reshape(-1, 1), X))
        amps = np.sqrt(X[:, 1] ** 2 + X[:, 2] ** 2) * 2 / X.shape[1]
        phases = np.arctan2(X[:, 1], -X[:, 2])

    except Exception as exception:  # Catches and raises errors- needs actual error listed.
        print('Excepted- search failed for omega = {:6.4f} rad/s.'.format(omega))
        print('Whatever error this is, please put into har_bal after the excepts (2 of them)')
        X = X0
        print(mask_constant)
        e = hb_err(X)
        if mask_constant:
            X = np.hstack((np.zeros_like(X[:, 0]).reshape(-1, 1), X))
        amps = np.sqrt(X[:, 1] ** 2 + X[:, 2] ** 2) * 2 / X.shape[1]
        phases = np.arctan2(X[:, 1], -X[:, 2])
        raise

Very much the same as the above question. This prints some information that could be put in the raise statement, but is not.

Requests for Maintainers

New Docstrings

The docstrings on the following functions are very weak and do not provide any useful insight into what the function does or how it works. Users are left to interpret what was originally a much messier single line of code.

  • duff_osc()
  • condense_fft()
  • condense_rfft()
  • expand_rfft()

New Variable Names in Specific Functions

The following functions were originally one line, but now are broken into multiple so as to be more readable. Single line functions are generally a bad idea, especially these, as their complexity makes them hard to read.

  • condense_fft()
  • condense_rfft()
  • expand_rfft()

New Variable Names

All code in this repository suffers from the same issue. The code was clearly written by a trained mathematician who knows how to program, and not a trained programmer who knows math. Variables such as x, e, x, m, x0, etc, are used extensively throughout the code. While these variables may translate nicely to math, unless those names are standardized across all applications of that math and every potential user is painfully familiar with them, names such as these hurt the readability of the code. Variable names should be descriptive and tell the user what they hold. For example, x is a valid name when doing an embarrassingly simple task, such as adding two values. However, if those two values have any sort of context associated with them, x is much better off being named after that context. When the context is complex, such as in this program, it is even more important to provide useful variable names so someone unfamiliar with the code can read and learn the algorithm. Code that can only be read by the author can only be maintained by the author.

Other Notes

Had I not been specifically requested by the author to review and format this code, I would not have done so. While I strongly believe in the majority of PEP, I believe that it is most important that the author of the code, and no one else, "PEP-ify" their code. There is a PyCon presentation by Raymond Hettinger that goes into this in depth. I highly recommend watching. There is a very strong chance that in making this code more readable, I broke something without realizing it. This should be expected, as I am unfamiliar with the code. However, without formatting the code first, anyone would find it hard to understand. For this reason I strongly emphasize that THIS PULL REQUEST SHOULD NOT BE MERGED WITHOUT EXTENSIVE TESTING AND REVIEW BY THE AUTHOR.

Mostly just standardized newlines after docstrings
Standardized '' strings, instead of "" or """"""

Cleaned an unnecessary line break I missed in last commit

Some small grammatical fixes

Added fstrings when applicable
Mostly just newlines to break up if/else
Removed unnecessary declaration

Commented old variable name for light documentation

Docstring needed
Broke out 3 vars to make function more than one line of code for the sake of readability. Variable names manually generated from what they appear to do. Requires author to rename
Pulled var from return var declaration to improve readability. Removed immedate return of new var. Requires rename of extracted var by author
Extracted data_type, empty_array, and data variables to improve readability and reduce the amount of work done on a single line
Comparing a single value to a set makes it easier to add more values to compare to in the future. Also added a .lower() to handle input better
@CodingPenguin1
Copy link
Author

Requesting review from @josephcslater

@sourcery-ai
Copy link

sourcery-ai bot commented Aug 15, 2021

Sourcery Code Quality Report

✅  Merging this PR will increase code quality in the affected files by 0.17%.

Quality metrics Before After Change
Complexity 13.77 🙂 13.60 🙂 -0.17 👍
Method Length 92.00 🙂 91.19 🙂 -0.81 👍
Working memory 12.45 😞 12.45 😞 0.00
Quality 46.40% 😞 46.57% 😞 0.17% 👍
Other metrics Before After Change
Lines 1284 1271 -13
Changed files Quality Before Quality After Quality Change
mousai/har_bal.py 46.40% 😞 46.57% 😞 0.17% 👍

Here are some functions in these files that still need a tune-up:

File Function Complexity Length Working Memory Quality Recommendation
mousai/har_bal.py hb_freq 28 😞 671 ⛔ 18 ⛔ 16.11% ⛔ Refactor to reduce nesting. Try splitting into smaller methods. Extract out complex expressions
mousai/har_bal.py hb_time 21 😞 468 ⛔ 15 😞 23.93% ⛔ Refactor to reduce nesting. Try splitting into smaller methods. Extract out complex expressions
mousai/har_bal.py hb_freq.hb_err 9 🙂 241 ⛔ 14 😞 40.35% 😞 Try splitting into smaller methods. Extract out complex expressions
mousai/har_bal.py function_to_mousai 16 🙂 143 😞 11 😞 46.91% 😞 Try splitting into smaller methods. Extract out complex expressions
mousai/har_bal.py hb_time.hb_err 7 ⭐ 167 😞 13 😞 49.11% 😞 Try splitting into smaller methods. Extract out complex expressions

Legend and Explanation

The emojis denote the absolute quality of the code:

  • ⭐ excellent
  • 🙂 good
  • 😞 poor
  • ⛔ very poor

The 👍 and 👎 indicate whether the quality has improved or gotten worse with this pull request.


Please see our documentation here for details on how these metrics are calculated.

We are actively working on this report - lots more documentation and extra metrics to come!

Help us improve this quality report!

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.

1 participant