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

redeboer · 2021-11-06T18:02:45Z

This is a follow-up demo to #22434 (now with yapf) and is not to be merged. See also #17287 (comment).

I played around a bit with the yapf configuration to improve the spacing around arithmetic operators and get it to look a bit more like black (see comparison):

sympy/setup.cfg

Lines 91 to 98 in 2d90dc1

    
           [yapf] 
        
           based_on_style = pep8 
        
           allow_split_before_dict_value = False 
        
           arithmetic_precedence_indication = True 
        
           dedent_closing_brackets = True 
        
           no_spaces_around_selected_binary_operators = *,/ 
        
           split_complex_comprehension = True

Release Notes

NO ENTRY

Applied formatting as follows: ```shell yapf -i -p $(git ls-files | grep -E "sympy/solvers/.*\.py") ```

sympy-bot · 2021-11-06T18:02:46Z

✅

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 is a follow-up demo to #22434 (now with [yapf](https://github.com/google/yapf)) and is not to be merged. See also https://github.com/sympy/sympy/issues/17287#issuecomment-962292806.**

I played around a bit with the yapf configuration to improve the spacing around arithmetic operators and get it to look a bit more like black (see [comparison](https://github.com/redeboer/sympy/compare/17287-apply-yapf-to-solvers...17287-apply-black-to-solvers?expand=1)):

https://github.com/sympy/sympy/blob/2d90dc1d91b1d16679bec2c0f94d49b836b5315b/setup.cfg#L91-L98

#### Release Notes

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

sympy-bot · 2021-11-06T18:02:48Z

🟠

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:

a222c7b:
- .pre-commit-config.yaml

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

smichr · 2021-11-06T18:28:17Z

sympy/solvers/ode/hypergeometric.py

    I0_num, I0_dem = I0.as_numer_denom()
    # collecting coeff of (x**2, x), of the standerd equation.
    # substituting (a-b) = s, (a+b) = r
-    dict_I0 = {x**2:s**2 - 1, x:(2*(1-r)*c + (r+s)*(r-s)), 1:c*(c-2)}
+    dict_I0 = {x**2: s**2 - 1, x: (2*(1-r)*c + (r+s)*(r-s)), 1: c*(c-2)}


it is reluctant to make changes in values of the dict; compare this to the addition of space arount the + at line 198 a few lines below

I think this is due to this option:

sympy/setup.cfg

Line 95 in 2d90dc1

arithmetic_precedence_indication = True

If you remove that option, it becomes:

dict_I0 = { x**2: s**2 - 1, x: (2*(1 - r)*c + (r + s)*(r - s)), 1: c*(c - 2) }

(line-breaks are just because this didn't fit within 79 anymore)

that looks better

Agreed. I switched it off (ba85647), but we have to check if that has undesired consequences elsewhere.

sympy/solvers/ode/hypergeometric.py

sympy/solvers/ode/riccati.py

github-actions · 2021-11-06T19:23:15Z

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]       [6247eefd]
-      5.56±0.02s          340±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).

oscarbenjamin · 2021-11-07T00:30:38Z

sympy/solvers/deutils.py

-        hints = classifier(eq, func, dict=True, ics=ics, xi=xi, eta=eta,
-        n=terms, x0=x0, hint=hint, prep=prep)
+        hints = classifier(
+            eq,
+            func,
+            dict=True,
+            ics=ics,
+            xi=xi,
+            eta=eta,
+            n=terms,
+            x0=x0,
+            hint=hint,
+            prep=prep
+        )


I don't really like this change. Maybe I would make a **opts dict or something but I'm not sure.

The line breaks are there because of the 79 column limit. If you set it to something like 100, it becomes:

hints = classifier( eq, func, dict=True, ics=ics, xi=xi, eta=eta, n=terms, x0=x0, hint=hint, prep=prep )

(I would stick with PEP8's 79, but it illustrates that arguments are put on one line if possible.)

The dedenting is the same effect as you noted here #22437 (comment). But yapf's default profile ("pep8") looks worse imo:

hints = classifier(eq, func, dict=True, ics=ics, xi=xi, eta=eta, n=terms, x0=x0, hint=hint, prep=prep)

Note that the same happens in function with many arguments:

sympy/sympy/solvers/deutils.py

Lines 143 to 152 in 357fae9

def _desolve(

eq,

func=None,

hint="default",

ics=None,

simplify=True,

*,

prep=True,

**kwargs

):

For function definitions, this style is quite helpful when type hints are added.

Type hints may change it, but I'm not a fan of this either. OK if it re-breaks the lines to have 80 characters, but having a separate line for each argument... (similar to the init-files)

oscarbenjamin · 2021-11-07T00:33:05Z

sympy/solvers/diophantine/diophantine.py

-                s = BinaryQuadratic(self.equation, free_symbols=[y, x]).solve(parameters=[t, u])
+                s = BinaryQuadratic(self.equation,
+                                    free_symbols=[y,
+                                                  x]).solve(parameters=[t, u])


This seems like an odd choice of line breaks

it's left -aligning elements at the opening bracket position if there is more than 1. It's kind of nice because you can scan vertically to see what the given brackets args are -- breadth first.

Tbh I don't understand the formatting here :S It looks like this also with the default yapf profile... But with split_before_expression_after_opening_paren = True, you get:

s = BinaryQuadratic( self.equation, free_symbols=[y, x] ).solve(parameters=[t, u])

See d864ba2

I guess this is another situation where an intermediate variable with a good name would be better

oscarbenjamin · 2021-11-07T00:34:26Z

sympy/solvers/diophantine/diophantine.py

+                        if (
+                            divisible(sqa*g*z0**2 + D*z0 + sqa*F, _c)
+                            and divisible(e*sqc*g*z0**2 + E*z0 + e*sqc*F, _c)
+                        ):


I don't understand why both black and yapf seem to prefer the unindented reverse smiley ):.

I think the issue page on which you commented recently at black mentioned about how they tokenize and without looking ahead or back, that is just the simplest thing to do. It also visually marks the range of the if

I don't understand why both black and yapf seem to prefer the unindented reverse smiley ):.

Haha reverse smiley. Actually, this is not the default yapf behaviour. It's a consequence of this option, which I added to mimic black:

sympy/setup.cfg

Lines 95 to 96 in 2d90dc1

arithmetic_precedence_indication = True

dedent_closing_brackets = True

If you remove that option, you get:

if (divisible(sqa*g*z0**2 + D*z0 + sqa*F, _c) and divisible(e*sqc*g*z0**2 + E*z0 + e*sqc*F, _c)): result.add((solve_x(z0), solve_y(z0)))

Even if the above is more 'concise', I think the split 'black' version is easier to understand at one glance.

Note: I had hoped split_before_logical_operator = True may improve the above a bit, but this seems to have no effect.

If you remove that option, you get:

if (divisible(sqa*g*z0**2 + D*z0 + sqa*F, _c) and divisible(e*sqc*g*z0**2 + E*z0 + e*sqc*F, _c)): result.add((solve_x(z0), solve_y(z0)))

That looks more like what I would have done when editing the code myself.

oscarbenjamin · 2021-11-07T00:35:15Z

sympy/solvers/diophantine/diophantine.py

-                    4*A*r*u*v + 4*A*D*(B*v + r*u + r*v - B*u) +
-                    2*A*4*A*E*(u - v) + 4*A*r*4*A*F)
+                    4*A*r*u*v + 4*A*D*(B*v + r*u + r*v - B*u) + 2*A*4*A*E*
+                    (u-v) + 4*A*r*4*A*F


This is odd. I'd rather break on + than *.

Well spotted, looks strange indeed... It seems that split_before_arithmetic_operator = True (c9054b1) helps here, and it's also more in line with black style:

eq = _mexpand( 4*A*r*u*v + 4*A*D*(B*v + r*u + r*v - B*u) + 2*A*4*A*E*(u-v) + 4*A*r*4*A*F )

oscarbenjamin · 2021-11-07T00:37:28Z

sympy/solvers/diophantine/diophantine.py

-                s = BinaryQuadratic(self.equation, free_symbols=var[::-1]).solve(parameters=[t, u])  # Interchange x and y
+                s = BinaryQuadratic(self.equation,
+                                    free_symbols=var[::-1]).solve(
+                                        parameters=[t, u]
+                                    )  # Interchange x and y


I guess this is the kind of situation where an intermediate variable would help.

Last time (above) it didn't break on the single argument. I guess it did now because of line length.

I guess this is the kind of situation where an intermediate variable would help.

True, although same as #22437 (comment), I don't really get why the formatting looks like this. But split_before_expression_after_opening_paren = True (d864ba2) improves it:

s = BinaryQuadratic( self.equation, free_symbols=var[::-1] ).solve(parameters=[t, u]) # Interchange x and y

Last time (above) it didn't break on the single argument. I guess it did now because of line length.

With black? Could be; black wraps at 88 by default.

oscarbenjamin · 2021-11-07T00:38:42Z

I haven't been through the whole diff but this looks a lot better than black because it has much better handling of mathematical expressions.

#22437 (comment)

redeboer · 2021-11-07T14:22:49Z

this looks a lot better than black because it has much better handling of mathematical expressions

I agree. With the recently added commits I'm also more satisfied with the diff.

So I think the current config is quite suitable. The next question is how to apply this formatting. For sure it has to be done by a bot, because this will result in a large amount of line changes ('contributions').

oscarbenjamin · 2021-11-07T21:36:36Z

The next question is how to apply this formatting

No, I think the next step is to go back to the original issue and ask for the opinions of others. I think that this is better than black but that doesn't mean I'm convinced that we should go with this (or indeed with any automatic formatting). There are many others whose inputs are needed and there needs to be something like consensus before adopting anything as invasive as this across the codebase.

Actually applying the changes is straight-forward once the basic idea is agreed and the details are worked out. We don't need a bot, we just need some people to make a bunch of PRs of manageable size and then others to review and merge those PRs.

redeboer · 2021-11-07T22:29:30Z

The next question is how to apply this formatting

No, I think the next step is to go back to the original issue

Sounds like a good next step. I think that question about applying formatting by a bot vs individuals will still have to be discussed as well though.

redeboer added 3 commits November 6, 2021 18:56

ci: add yapf configuration

2d90dc1

ci: add pre-commit hook for yapf

a222c7b

style: apply yapf formatting on sympy/solvers

357fae9

Applied formatting as follows: ```shell yapf -i -p $(git ls-files | grep -E "sympy/solvers/.*\.py") ```

redeboer changed the title ~~17287 apply yapf to solvers~~ [Demo PR] Apply yapf to sympy/solvers Nov 6, 2021

redeboer mentioned this pull request Nov 6, 2021

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

Closed

fix: ignore mypy errors

b20988e

redeboer changed the title ~~[Demo PR] Apply yapf to sympy/solvers~~ [Demo PR] Format sympy/solvers with yapf Nov 6, 2021

smichr reviewed Nov 6, 2021

View reviewed changes

sympy/solvers/ode/hypergeometric.py Outdated Show resolved Hide resolved

redeboer commented Nov 6, 2021

View reviewed changes

sympy/solvers/ode/riccati.py Outdated Show resolved Hide resolved

oscarbenjamin reviewed Nov 7, 2021

View reviewed changes

redeboer added 7 commits November 7, 2021 15:14

style: split before expression after opening parenthesis

d864ba2

style: split all top level comma separated values

a7134ab

style: remove space around // operator

f4c024c

#22437 (comment)

style: split before arithmetic operator

c9054b1

style: activate more "split before" options

68ffa20

style: remove arithmetic precedence indication

ba85647

#22437 (comment)

style: remove space after comma in single-element tuple

0384d1d

oscarbenjamin added the Code quality label Nov 10, 2021

github-actions bot added the Merge conflict label Oct 26, 2022

redeboer closed this by deleting the head repository Jan 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

redeboer commented Nov 6, 2021 •

edited

sympy-bot commented Nov 6, 2021 •

edited

sympy-bot commented Nov 6, 2021 •

edited

smichr Nov 6, 2021

redeboer Nov 6, 2021

redeboer Nov 6, 2021

smichr Nov 7, 2021

redeboer Nov 7, 2021

github-actions bot commented Nov 6, 2021

oscarbenjamin Nov 7, 2021

redeboer Nov 7, 2021

oscargus Nov 19, 2021

oscarbenjamin Nov 7, 2021

smichr Nov 7, 2021

redeboer Nov 7, 2021

oscarbenjamin Nov 7, 2021

oscarbenjamin Nov 7, 2021

smichr Nov 7, 2021

redeboer Nov 7, 2021

oscarbenjamin Nov 7, 2021

oscarbenjamin Nov 7, 2021

redeboer Nov 7, 2021

oscarbenjamin Nov 7, 2021

smichr Nov 7, 2021

redeboer Nov 7, 2021

oscarbenjamin commented Nov 7, 2021

redeboer commented Nov 7, 2021

oscarbenjamin commented Nov 7, 2021

redeboer commented Nov 7, 2021

	[yapf]
	based_on_style = pep8

	allow_split_before_dict_value = False
	arithmetic_precedence_indication = True
	dedent_closing_brackets = True
	no_spaces_around_selected_binary_operators = *,/
	split_complex_comprehension = True

	def _desolve(
	eq,
	func=None,
	hint="default",
	ics=None,
	simplify=True,
	*,
	prep=True,
	**kwargs
	):

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

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

Conversation

redeboer commented Nov 6, 2021 • edited

Release Notes

sympy-bot commented Nov 6, 2021 • edited

sympy-bot commented Nov 6, 2021 • edited

🟠

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oscarbenjamin commented Nov 7, 2021

redeboer commented Nov 7, 2021

oscarbenjamin commented Nov 7, 2021

redeboer commented Nov 7, 2021

redeboer commented Nov 6, 2021 •

edited

sympy-bot commented Nov 6, 2021 •

edited

sympy-bot commented Nov 6, 2021 •

edited