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

Mathematical expressions become unreadable #1984

Open
jkochNU opened this issue Feb 13, 2021 · 2 comments
Open

Mathematical expressions become unreadable #1984

jkochNU opened this issue Feb 13, 2021 · 2 comments
Labels
T: style What do we want Blackened code to look like?

Comments

@jkochNU
Copy link

jkochNU commented Feb 13, 2021

Request: minimal support for reasonable formatting of mathematical expressions
Readability of mathematical expressions is critical for numerical code, and this is vitally dependent on reasonable code formatting. Black appears to have no sense of this issue, and produces code formatting that makes mathematical expressions not just obscure, but unreadable. I (developer of scqubits) love the idea of uniform code formatting. I also love the idea of enforcing uniformity by not providing options for formatting variations. The latter, however, can only work if the choices black makes fulfill minimal requirements of readability. If there is any interest within the black development team to address this issue, I would be happy to contribute and advise on reasonable formatting of mathematical expressions.

Evidence of the above

      def spectral_density(omega):
            therm_ratio = calc_therm_ratio(omega, T)
            s = (
                2
                * self.EL
                / q_ind_fun(omega)
                * (1 / np.tanh(0.5 * np.abs(therm_ratio)))
                / (1 + np.exp(-therm_ratio))
            )

Reasonable style

     def spectral_density(omega):
            therm_ratio = calc_therm_ratio(omega, T)
            s = (
                2 * self. EC / q_cap_fun(omega)
                * (1 / np.tanh(0.5 * np.abs(therm_ratio))) / (1 + np.exp(-therm_ratio))
            )

The latter fits into the default 88 character line width just fine.

@jkochNU jkochNU added the T: style What do we want Blackened code to look like? label Feb 13, 2021
@jkochNU jkochNU changed the title Mathematical expression become unreadable Mathematical expressions become unreadable Feb 13, 2021
@YodaEmbedding
Copy link

YodaEmbedding commented Feb 17, 2021

Given that the most elegant presentation of a mathematical expression is highly variable, I think it would be best if black simply trusted the developer on this. (Except for lines that exceed the line length or break PEP-8 compliance.) This might involve some sort of heuristic detection of what constitutes a "math expression". Expressions that contain the math operators + - * / ** or functions such as np.* math.* abs exp sin floor are usually good candidates.

So, the following code samples should all be stable, i.e., black should not alter these:

result = alpha * beta**zeta + gamma * delta + epsilon * sigma / 2

result = alpha*beta**zeta + gamma*delta + epsilon*sigma/2

result = (
    alpha * beta ** zeta
    + gamma * delta
    + epsilon * sigma / 2
)

result = (
    alpha*beta**zeta
    + gamma*delta
    + epsilon*sigma/2
)

...One might claim that the second sample above is the least readable, but readability of math expressions is often determined by the variable lengths. For instance, with shorter variable lengths:

result = a * b ** z + g * d + e * s / 2

result = a*b**z + g*d + e*s/2

result = (
    a * b ** z
    + g * d
    + e * s / 2
)

result = (
    a*b**z
    + g*d
    + e*s/2
)

I think the second sample is actually the most readable, and also the most succinct.

black should do the minimum required for PEP-8 compliance, e.g., messing around with ** if necessary. Even doing that might be controversial and non-desirable among the scientific development community.

Also slightly related: #148

@fpdotmonkey
Copy link

fpdotmonkey commented May 3, 2021

Just want to add some thoughts to this.

A long expression like this need to be wrapped over several line

area_ratio = 1 / mach_number * ((1 + 1/2 * (boltzmann_constant - 1) * mach_number ** 2) / (1/2 * (boltzmann_constant + 1))) ** (1/2 * (boltzmann_constant+1) / (boltzmann_constant-1))

black does this, but it's rather ugly.

area_ratio = (
    1
    / mach_number
    * (
        (1 + 1 / 2 * (boltzmann_constant - 1) * mach_number ** 2)
        / (1 / 2 * (boltzmann_constant + 1))
    )
    ** (1 / 2 * (boltzmann_constant + 1) / (boltzmann_constant - 1))
)

Specifically, 1 is on a line on its own, and the exponent should be further to the right. 1 / mach_number would be read as a single term meaning "the inverse of the mach number" as a single , so the line should reflect that and have exactly that format.

The exponent is tabbed to the same level as the other lines, but it doesn't apply at the same level as the other operators, indeed it only applies to the previous term. So I propose that the exponent be tabbed to the level of the first parenthesis of the term that it applies to.

So this would be a more desirable format.

area_ratio = (
    1 / mach_number
    * (
        (1 + 1 / 2 * (boltzmann_constant - 1) * mach_number ** 2)
        / (1 / 2 * (boltzmann_constant + 1))
    )
      ** (1 / 2 * (boltzmann_constant + 1) / (boltzmann_constant - 1))
)

Essentially, you should be able to visually parse the order of operations. You can clearly see in the above that you've got the inverse of a number multiplying a fraction, and that fraction is raised to an exponent

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: style What do we want Blackened code to look like?
Projects
None yet
Development

No branches or pull requests

3 participants