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

PEP8 / Black style makes some code less readable #235

Open
paulthebaker opened this issue Oct 14, 2020 · 4 comments
Open

PEP8 / Black style makes some code less readable #235

paulthebaker opened this issue Oct 14, 2020 · 4 comments
Assignees

Comments

@paulthebaker
Copy link
Member

Complicated equations are made less, not more, readable by following the PEP8 style that black enforces.

You can see this type of thing in signals.utils, where some of the most complicated mathematical expressions live:
original

    dFdt = 48 / (5*np.pi*mc**2) * (2*np.pi*mc*F)**(11/3) * \
        (1 + 73/24*e**2 + 37/96*e**4) / ((1-e**2)**(7/2))

after black

    dFdt = (
        48
        / (5 * np.pi * mc ** 2)
        * (2 * np.pi * mc * F) ** (11 / 3)
        * (1 + 73 / 24 * e ** 2 + 37 / 96 * e ** 4)
        / ((1 - e ** 2) ** (7 / 2))
    )

Does anyone know how to configure black to ignore certain rules?

The pre-black linter was configured to ignore "space around operator" errors, so things like

2*10**logA + B/C

wouldn't be flagged. The linter wanted it to be 2 * 10 ** logA + B / C which I find much harder to parse by eye.

another example

In PR #228 this block (which could be a bit better) was originally this:

return (
    sum(
        (params[efac.name] if efac.name in params else efac.value) * mask
        for efac, mask in zip(self._dmefacs, self._dmefac_masks)
        )**2
        * self._dmerr**2 +
        (10**sum(
            (params[equad.name] if equad.name in params else equad.value) * mask
             for equad, mask in zip(self._log10_dmequads, self._log10_dmequad_masks)
        ))**2
)**0.5

It becomes this (which is harder for me to follow):

return (
    sum(
        (params[efac.name] if efac.name in params else efac.value) * mask
        for efac, mask in zip(self._dmefacs, self._dmefac_masks)
    )
    ** 2
    * self._dmerr ** 2
    + (
        10
        ** sum(
            (params[equad.name] if equad.name in params else equad.value) * mask
            for equad, mask in zip(self._log10_dmequads, self._log10_dmequad_masks)
        )
    )
    ** 2
) ** 0.5
@kayhangultekin
Copy link

kayhangultekin commented Oct 14, 2020 via email

@scottransom
Copy link
Member

OMG, whoever wrote that return statement should be made to manually re-lint all the code..... That is atrocious (black or no black).

@paulthebaker
Copy link
Member Author

I guess we could tell it to ignore whole files. At this point since black reformatted everything, it'd be a huge pain to manually re-lint things.

I guess the solution is to break complicated equations into parts, storing individual terms in variables. That would mean the whole code would be built from simpler expressions.

@paulthebaker
Copy link
Member Author

OMG, whoever wrote that return statement should be made to manually re-lint all the code..... That is atrocious (black or no black).

I didn't write it, but I merged it... so I'm possibly more to blame 🤦

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants