[Demo PR] Apply black formatting to sympy/solvers #22434

redeboer · 2021-11-06T12:08:02Z

This PR serves as a demo and test only and is not to be merged. See #17287 (comment).

Formatting was applied over a few different commits, so changes can best be discussed through its commit history.

Release Notes

NO ENTRY

As recommended for existing projects, string normalization is switched off: https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#strings Other config options, such as line width, are explained here: https://black.readthedocs.io/en/stable/usage_and_configuration/the_basics.html#configuration-format A config for isort was added in to ensure that isort is applied in agreement with black once/if isort is applied

Pre-commit allows for incremental changes and pins the version of the tool that is applied. Install as folllows: python3 -m pip install pre-commit pre-commit install

See #17287 (comment) Applied formatting as follows: # python3 -m pip install black==21.10b0 black sympy/solvers

https://black.readthedocs.io/en/stable/the_black_code_style/future_style.html#improved-string-processing

sympy-bot · 2021-11-06T12:08:04Z

✅

Hi, I am the SymPy bot (v162). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

No release notes entry will be added for this pull request.

Click here to see the pull request description that was parsed.

**This PR serves as a demo and test only and is not to be merged. See https://github.com/sympy/sympy/issues/17287#issuecomment-962292806.**

Formatting was applied over a few different commits, so changes can best be discussed through [its commit history](https://github.com/sympy/sympy/pull/22434/commits).

#### Release Notes

<!-- BEGIN RELEASE NOTES -->
NO ENTRY
<!-- END RELEASE NOTES -->

sympy-bot · 2021-11-06T12:08:06Z

🟠

Hi, I am the SymPy bot (v162). I've noticed that some of your commits add or delete files. Since this is sometimes done unintentionally, I wanted to alert you about it.

This is an experimental feature of SymPy Bot. If you have any feedback on it, please comment at sympy/sympy-bot#75.

The following commits add new files:

f0ad6ca:
- pyproject.toml
ea6e4eb:
- .pre-commit-config.yaml

If these files were added/deleted on purpose, you can ignore this message.

redeboer · 2021-11-06T12:10:14Z

Ahh missed this, see also #22432

redeboer · 2021-11-06T12:14:09Z

sympy/solvers/diophantine/tests/test_diophantine.py

+    assert diop_solve(0 * x - y - 5) == (-5,)
+    assert diop_solve(3 * y + 2 * x - 5) == (3 * t_0 - 5, -2 * t_0 + 5)
+    assert diop_solve(2 * x - 3 * y - 5) == (3 * t_0 - 5, 2 * t_0 - 5)
+    assert diop_solve(-2 * x - 3 * y - 5) == (3 * t_0 + 5, -2 * t_0 - 5)
+    assert diop_solve(7 * x + 5 * y) == (5 * t_0, -7 * t_0)
+    assert diop_solve(2 * x + 4 * y) == (2 * t_0, -t_0)
+    assert diop_solve(4 * x + 6 * y - 4) == (3 * t_0 - 2, -2 * t_0 + 2)
+    assert diop_solve(4 * x + 6 * y - 3) == (None, None)
+    assert diop_solve(0 * x + 3 * y - 4 * z + 5) == (4 * t_0 + 5, 3 * t_0 + 5)
+    assert diop_solve(4 * x + 3 * y - 4 * z + 5) == (
+        t_0,
+        8 * t_0 + 4 * t_1 + 5,
+        7 * t_0 + 3 * t_1 + 5,
+    )
+    assert diop_solve(4 * x + 3 * y - 4 * z + 5, None) == (0, 5, 5)
+    assert diop_solve(4 * x + 2 * y + 8 * z - 5) == (None, None, None)
+    assert diop_solve(5 * x + 7 * y - 2 * z - 6) == (
+        t_0,
+        -3 * t_0 + 2 * t_1 + 6,
+        -8 * t_0 + 7 * t_1 + 18,
+    )
+    assert diop_solve(3 * x - 6 * y + 12 * z - 9) == (2 * t_0 + 3, t_0 + 2 * t_1, t_1)
+    assert diop_solve(6 * w + 9 * x + 20 * y - z) == (
+        t_0,
+        t_1,
+        t_1 + t_2,
+        6 * t_0 + 29 * t_1 + 20 * t_2,
+    )


@oscarbenjamin this is probably what you meant in #17287 (comment). See also #17287 (comment)

Is it possible to configure black so that it does not force spaces around all operators?

Not being able to visually distinguish operator precedence is not good. If this is forced by black then that's a deal-breaker as far as I'm concerned.

It seems not:
https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html
Black is intentionally hardly configurable. (I personally prefer that, but can see that is problematic for math-oriented code.) A possible way out is to put fmt:on/off around the code block.

Yapf is more configurable and seems to be able to handle this operator style:
https://github.com/google/yapf#knobs
Afaik yapfs is less popular, but was mentioned in the original issue #17287 (comment)

Then I don't think we can use black. I commented here: psf/black#538 (comment)

A post-process that closes up space on desired operators wouldn't be too hard, would it?

Then I don't think we can use black. I commented here: psf/black#538 (comment)

Thanks for investigating this and reporting it upstream!

A post-process that closes up space on desired operators wouldn't be too hard, would it?

I wouldn't go for a post-process. The motivation to use black is to have standardised formatting, so if there is some post-formatting, you remove the reason of using black.

oscarbenjamin · 2021-11-06T12:54:03Z

sympy/solvers/diophantine/tests/test_diophantine.py

 def test_transformation_to_normal():
-    assert is_normal_transformation_ok(x**2 + 3*y**2 + z**2 - 13*x*y - 16*y*z + 12*x*z)
-    assert is_normal_transformation_ok(x**2 + 3*y**2 - 100*z**2)
-    assert is_normal_transformation_ok(x**2 + 23*y*z)
-    assert is_normal_transformation_ok(3*y**2 - 100*z**2 - 12*x*y)
-    assert is_normal_transformation_ok(x**2 + 23*x*y - 34*y*z + 12*x*z)
-    assert is_normal_transformation_ok(z**2 + 34*x*y - 23*y*z + x*z)
-    assert is_normal_transformation_ok(x**2 + y**2 + z**2 - x*y - y*z - x*z)
-    assert is_normal_transformation_ok(x**2 + 2*y*z + 3*z**2)
-    assert is_normal_transformation_ok(x*y + 2*x*z + 3*y*z)
-    assert is_normal_transformation_ok(2*x*z + 3*y*z)
+    assert is_normal_transformation_ok(
+        x ** 2 + 3 * y ** 2 + z ** 2 - 13 * x * y - 16 * y * z + 12 * x * z
+    )
+    assert is_normal_transformation_ok(x ** 2 + 3 * y ** 2 - 100 * z ** 2)
+    assert is_normal_transformation_ok(x ** 2 + 23 * y * z)
+    assert is_normal_transformation_ok(3 * y ** 2 - 100 * z ** 2 - 12 * x * y)
+    assert is_normal_transformation_ok(x ** 2 + 23 * x * y - 34 * y * z + 12 * x * z)
+    assert is_normal_transformation_ok(z ** 2 + 34 * x * y - 23 * y * z + x * z)
+    assert is_normal_transformation_ok(x ** 2 + y ** 2 + z ** 2 - x * y - y * z - x * z)
+    assert is_normal_transformation_ok(x ** 2 + 2 * y * z + 3 * z ** 2)
+    assert is_normal_transformation_ok(x * y + 2 * x * z + 3 * y * z)
+    assert is_normal_transformation_ok(2 * x * z + 3 * y * z)


This is horrible

github-actions · 2021-11-06T13:37:27Z

Benchmark results from GitHub Actions

Lower numbers are good, higher numbers are bad. A ratio less than 1
means a speed up and greater than 1 means a slowdown. Green lines
beginning with + are slowdowns (the PR is slower then master or
master is slower than the previous release). Red lines beginning
with - are speedups.

Significantly changed benchmark results (PR vs master)

Significantly changed benchmark results (master vs previous release)

       before           after         ratio
     [907895ac]       [3a49fc4d]
-      4.97±0.03s          310±4ms     0.06  polygon.PolygonArbitraryPoint.time_bench01

Full benchmark results can be found as artifacts in GitHub Actions
(click on checks at the top of the PR).

smichr · 2021-11-06T15:46:23Z

I wonder why this PR didn't raise an error like the other one did in #22432.

Maybe you used a more recent version of black. My imports didn't move the ignore to the first line (or else I hand edited it...I don't remember doing so, however).

from sympy.simplify import (simplify, collect, powsimp, posify,  # type: ignore
    powdenest, nsimplify, denom, logcombine, sqrtdenest, fraction,
    separatevars)

became

from sympy.simplify import (
    simplify,
    collect,
    powsimp,
    posify,  # type: ignore
    powdenest,
    nsimplify,
    denom,
    logcombine,
    sqrtdenest,
    fraction,
    separatevars,
)

Your version kept 2 ignores: one on the import line and one with the posify line.

redeboer · 2021-11-06T17:59:32Z

I wonder why this PR didn't raise an error like the other one did in #22432.

Probably because we formatted different modules? There are also some lint ignores that need to be moved to the right place.

redeboer · 2021-11-06T18:05:58Z

So I start to think black is indeed not a viable option for sympy, given the spacing around arithmetic operators. As an alternative, yapf with some additional configuration for those operators is illustrated in #22437.

redeboer added 7 commits November 6, 2021 13:04

ci: add black as pre-commit hook

ea6e4eb

Pre-commit allows for incremental changes and pins the version of the tool that is applied. Install as folllows: python3 -m pip install pre-commit pre-commit install

style: apply black formatting to sympy/solvers

66528eb

See #17287 (comment) Applied formatting as follows: # python3 -m pip install black==21.10b0 black sympy/solvers

ci: activate black string normalization

ff2c81c

style: apply black formatting to sympy/solvers

196c7eb

See #17287 (comment) Applied formatting as follows: # python3 -m pip install black==21.10b0 black sympy/solvers

ci: activate experimental string processing

420a5f4

https://black.readthedocs.io/en/stable/the_black_code_style/future_style.html#improved-string-processing

style: apply experimental string processing to sympy/solvers

0cdd862

redeboer mentioned this pull request Nov 6, 2021

Unified coding (spacing) style for sympy. #17287

Open

redeboer commented Nov 6, 2021

View reviewed changes

fix: ignore mypy errors

afee03a

oscarbenjamin reviewed Nov 6, 2021

View reviewed changes

oscarbenjamin mentioned this pull request Nov 6, 2021

Allow no space between operator ** psf/black#538

Closed

smichr mentioned this pull request Nov 6, 2021

black demonstration #22432

Closed

redeboer mentioned this pull request Nov 6, 2021

[Demo PR] Format sympy/solvers with yapf #22437

Closed

redeboer closed this Nov 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Demo PR] Apply black formatting to sympy/solvers #22434

[Demo PR] Apply black formatting to sympy/solvers #22434

redeboer commented Nov 6, 2021 •

edited

sympy-bot commented Nov 6, 2021 •

edited

sympy-bot commented Nov 6, 2021 •

edited

redeboer commented Nov 6, 2021

redeboer Nov 6, 2021

oscarbenjamin Nov 6, 2021

redeboer Nov 6, 2021

oscarbenjamin Nov 6, 2021

smichr Nov 6, 2021

redeboer Nov 6, 2021

oscarbenjamin Nov 6, 2021

github-actions bot commented Nov 6, 2021 •

edited

smichr commented Nov 6, 2021 •

edited

redeboer commented Nov 6, 2021

redeboer commented Nov 6, 2021 •

edited

[Demo PR] Apply black formatting to sympy/solvers #22434

[Demo PR] Apply black formatting to sympy/solvers #22434

Conversation

redeboer commented Nov 6, 2021 • edited

Release Notes

sympy-bot commented Nov 6, 2021 • edited

sympy-bot commented Nov 6, 2021 • edited

🟠

redeboer commented Nov 6, 2021

redeboer Nov 6, 2021

Choose a reason for hiding this comment

oscarbenjamin Nov 6, 2021

Choose a reason for hiding this comment

redeboer Nov 6, 2021

Choose a reason for hiding this comment

oscarbenjamin Nov 6, 2021

Choose a reason for hiding this comment

smichr Nov 6, 2021

Choose a reason for hiding this comment

redeboer Nov 6, 2021

Choose a reason for hiding this comment

oscarbenjamin Nov 6, 2021

Choose a reason for hiding this comment

github-actions bot commented Nov 6, 2021 • edited

smichr commented Nov 6, 2021 • edited

redeboer commented Nov 6, 2021

redeboer commented Nov 6, 2021 • edited

redeboer commented Nov 6, 2021 •

edited

sympy-bot commented Nov 6, 2021 •

edited

sympy-bot commented Nov 6, 2021 •

edited

github-actions bot commented Nov 6, 2021 •

edited

smichr commented Nov 6, 2021 •

edited

redeboer commented Nov 6, 2021 •

edited