CodingPenguin1 commented 3 years ago

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.

CodingPenguin1 commented 3 years ago

Requesting review from @josephcslater

sourcery-ai[bot] commented 3 years ago

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!

josephcslater / mousai

Linting and Formatting #19