Attempt to make SPEC 12 complete and unambiguous by mdhaber · Pull Request #1 · tupui/specs

mdhaber · 2024-07-14T17:05:11Z

I think the only differences with the examples you have shown are:

dfdx = sign*(-2*x + 2*y + 2)

becomes

dfdx = sign * (-2*x + 2*y + 2)

and

c_i1j = (
    1./n**2.
    *np.prod(
        0.5*(2. + abs(z_ij[i1, :]) + abs(z_ij) - abs(z_ij[i1, :] - z_ij)), axis=1
    )
)

becomes

c_i1j = (1./n**2. * np.prod(0.5 * (2. + abs(z_ij[i1, :]) 
                                   + abs(z_ij) - abs(z_ij[i1, :] - z_ij)), axis=1))

(although I haven't said anything that would contradict Black's rules about parentheses and line breaks, I don't think, so there is some flexibility to include the surrounding parentheses on separate lines.)

mdhaber

Some self review.

Please also look for examples in which the rules are ambiguous or lead to unfortunate formatting!

mdhaber · 2024-07-14T17:14:43Z

-)
+An "explicit" expression is a code expression enclosed within parentheses or
+otherwise syntactically separated from other expressions (i.e. by code other
+than operators, whitespace, literals, or variables). For example, in the list


I'm not sure if this list is complete.

mdhaber · 2024-07-14T17:32:58Z

+`i + 1` are explicit subexpressions of the expression `range(1, i + 1)`. `i` and
+`1` are "implicit" subexpressions of `i + 1`: they could be written as explicit
+subexpressions `(i)` and `(1)` without affecting the order  of operations, but they
+are not explicit as written.


I think this concept might still need refinement to make the rules unambiguous, but let's see if others find ambiguities before adding unnecessarily.

I was not sure about how detailed this should be as I would imagine it could depend on how formatter operate internally.

To some extent, I intended to write this to communicate the rules with the developers of Black and ruff. If it does not need to be as precise for readers, the formal definitions can be moved to the postscript for the interested reader.

Co-authored-by: Matt Haberland <mhaberla@calpoly.edu>

mdhaber · 2024-07-14T18:22:25Z

Should probably also add a table of all operators with priority level. I forgot about several of them before looking it up. Update: done.

tupui

Thanks Matt!

tupui · 2024-07-14T18:17:06Z

+`i + 1` are explicit subexpressions of the expression `range(1, i + 1)`. `i` and
+`1` are "implicit" subexpressions of `i + 1`: they could be written as explicit
+subexpressions `(i)` and `(1)` without affecting the order  of operations, but they
+are not explicit as written.


I was not sure about how detailed this should be as I would imagine it could depend on how formatter operate internally.

tupui · 2024-07-14T18:24:30Z

+SPEC endeavor to use them where practical. These rules are intended to respect and
+complement the PEP8 rules (relevant sections includes [id20](https://www.python.org/dev/peps/pep-0008/#id20) 
+and [id28](https://www.python.org/dev/peps/pep-0008/#id28)).


What is missing from this intro is if the rules are to be applied sequentially or if they are independent.

Neither? All are supposed to be satisfied simultaneously.

tupui · 2024-07-14T18:25:53Z

+   i.e., do not add extraneous parentheses. For example, prefer `u**v + y**z`
+   over `(u**v) + (y**z)`, and prefer `x + y + z` over `(x + y) + z`.
+1. Always use the `**` operator and unary `+`, `-`, and `~` operators *without*
+   surrounding whitespace. For example, prefer `-x**4` over `- (x ** 4)`. 


prefer: I think we should be more direct, these are rules and there should be no ambiguity. And you have rule 10 to break things if needed.

With such document, the norm is to use something like the RFC-2119 https://datatracker.ietf.org/doc/html/rfc2119

Here and bellow with things like "should". This makes it too ambiguous to me as well. There should be one and only one was to do things. That's the goal of this doc, that it can be used by any formatter and it would give the same output consistently.

We can change "prefer... over" to "use... instead of"?

I read RFC-2119 to mean that "should" is the prefered use because these are not absolute requirements. There are exceptions as defined by rule 10.

@lucascolley what do you think on this point?

tupui · 2024-07-15T19:24:31Z

+     horizontal bar. For example, prefer `z = t/v * x/y` over `z = t / v * x / y`
+     if this would  be written mathematically as the product of two fractions,
+     e.g. $\frac{t}{v} \cdot \frac{x}{y}.
+5. Considering the previous rules, only `**`, `*`, `/`, and the unary `+`, `-`, and `~`


Considering the previous rules

Linked to my comment saying that we should say if rules are to be applied in a specific order or not.

I also don't get the "only". To me it looks like all operators are listed.

Note the distinction between unary and binary +/- and the existence of many other operators.

tupui · 2024-07-15T19:30:20Z

+   - The expressions `--x` and `-~x` would be implicit subexpressions without spaces
+     containing more than one unary operator. The former can be simplified to `+x` or
+     simply `x`, and the latter requires explicit parentheses, i.e. `-(~x)`.


We can leave this for now and see what formatting folks tell us about the feasibility of such thing. Like if we go down that route, why not also recommend some operations vs other for precision issue? All that could seem a bit too far for a formatter though.

Sounds good. Note that these are all just examples of the implications of the two simple rules above that I thought were worth pointing out specifically.

tupui · 2024-07-15T19:35:30Z

+   (t 
+    + (w + (x + (y + z)))))


We could clarify that we don't specify how parenthesis should be handled and it's only about where to break on mathematical operations.

See #1 (comment)

tupui · 2024-07-15T19:39:22Z

+    * z)
+   ```
+   If there are multiple candidates, include the break at the first opportunity.
+9. The packages or subpackages from which mathematical functions and constants are used


This one is out of scope to me. Also linter/formatters have different rules for imports and typically would not allow to import * and would complain about shadowing, etc.

In general this is covered by the famous Namespaces are one honking great idea -- let's do more of those!. And long live import scipy as sp (which is getting loads of upvote on SO 😜 )

It was a last minute addition. I thought it might be worth broadening the scope a bit in an effort to make mathematical code more clear. Can we see what others think?

Note that it doesn't specifically disallow from _ import * or specify how namespaces should be used in general. It only requires that the namespace be unambiguous (if there are mathematical functions from multiple packages used in the same file) and show one way of achieving that as an example.

I agree that this guidance is useful, however it does seem to be a bit of a digression from the main topic of the SPEC. Could it become a separate proposal, or something like an appendix?

mdhaber · 2024-07-15T19:51:32Z

+`i + 1` are explicit subexpressions of the expression `range(1, i + 1)`. `i` and
+`1` are "implicit" subexpressions of `i + 1`: they could be written as explicit
+subexpressions `(i)` and `(1)` without affecting the order  of operations, but they
+are not explicit as written.


To some extent, I intended to write this to communicate the rules with the developers of Black and ruff. If it does not need to be as precise for readers, the formal definitions can be moved to the postscript for the interested reader.

mdhaber · 2024-07-15T19:56:42Z

+   i.e., do not add extraneous parentheses. For example, prefer `u**v + y**z`
+   over `(u**v) + (y**z)`, and prefer `x + y + z` over `(x + y) + z`.
+1. Always use the `**` operator and unary `+`, `-`, and `~` operators *without*
+   surrounding whitespace. For example, prefer `-x**4` over `- (x ** 4)`. 


We can change "prefer... over" to "use... instead of"?

I read RFC-2119 to mean that "should" is the prefered use because these are not absolute requirements. There are exceptions as defined by rule 10.

mdhaber · 2024-07-15T19:58:00Z

+     horizontal bar. For example, prefer `z = t/v * x/y` over `z = t / v * x / y`
+     if this would  be written mathematically as the product of two fractions,
+     e.g. $\frac{t}{v} \cdot \frac{x}{y}.
+5. Considering the previous rules, only `**`, `*`, `/`, and the unary `+`, `-`, and `~`


Note the distinction between unary and binary +/- and the existence of many other operators.

mdhaber · 2024-07-15T20:02:02Z

+   - The expressions `--x` and `-~x` would be implicit subexpressions without spaces
+     containing more than one unary operator. The former can be simplified to `+x` or
+     simply `x`, and the latter requires explicit parentheses, i.e. `-(~x)`.


Sounds good. Note that these are all just examples of the implications of the two simple rules above that I thought were worth pointing out specifically.

mdhaber · 2024-07-15T20:15:16Z

+    * z)
+   ```
+   If there are multiple candidates, include the break at the first opportunity.
+9. The packages or subpackages from which mathematical functions and constants are used


It was a last minute addition. I thought it might be worth broadening the scope a bit in an effort to make mathematical code more clear. Can we see what others think?

Note that it doesn't specifically disallow from _ import * or specify how namespaces should be used in general. It only requires that the namespace be unambiguous (if there are mathematical functions from multiple packages used in the same file) and show one way of achieving that as an example.

mdhaber · 2024-08-11T02:54:50Z

@lucascolley I saw your post on the forum and I thought I'd draw your attention to this. It looks like you'll have some different opinions based on what you wrote - since we're trying to come up with a complete specification, please think about how they fit into the whole.

lucascolley

Overall LGTM! I haven't checked that I would agree with every possible instance of these rules, but I would certainly be happy with them being implemented, modulo my inline disagreement.

lucascolley · 2024-08-11T09:28:29Z

+     operator due to the presence of the lower-priority addition operator. However,
+     this would lead to `t**v*x**y` being an implicit subexpression without spaces
+     containing more than one `**` operator. This code would be executed as 
+    `(t**v)*(x**y) + z`, but the explicit parentheses should be included for clarity.


lucascolley · 2024-08-11T09:29:55Z

+4. Typically, surround MD operators with whitespace, except in the following situations.
+   - When there are lower-priority operators (namely AS) within the same compound
+     expression. For example, prefer `z = -x * y**t` over `z = -x*y**t`, but
+     prefer `z = w + x*y**t` over `z = w + x * y**t` due to the presence of the


As below, I think there should be parentheses here to distinguish between * and **:

z = w + x*(y**t)

Also, how would the rules format w * x * y**z + u ** v * q?

I think my preference is:

(w * x * y**z) + (u**v * q)

But I would accept:

(w*x*(y**z)) + ((u**v)*q)

Here, the presence of the lower priority operator + in the expression means that there will be no space around the multiplication operators. Also, ** is supposed to appear at the end of "implicit subexpression without spaces". So it would be w*x*y**z + q*u**v.

This is intended to be reminiscent of mathematics in which we would write
$wxy^z + qu^v$.

All operators with priority lower than PEMDAS operators need parentheses. But I didn't require parentheses in expressions that involve, say, multiplication and addition because I think people are familiar with the order of operations of such operators from elementary school and it is natural to them to omit the parentheses . I posit that it is just as natural to write w*x + y*z instead of (w*x) + (y*z) as it is to write w**x * y**z instead of (w**x) * (y**z). Similarly, I think putting ** at the end of subexpressions without spaces is enough to distinguish it visually from *.

The rules would be a bit simpler to express, actually, if we did require those parentheses like you want. But I'm not sure if that's very common in real code. I had ChatGPT write me a script to extract long math operations from the codebase.. maybe I'll resurrect that to see.

What about w*x*(y**z) + q*(u**v)? I would be happy with that.

We need a stronger rationale than what makes one of us happy to change that : )

Would you really write $qu^v$ as $q(u^v)$?

And you'll say no, but the superscript stands out so it's readable. Then I say ok, well I think the ** at the end stands out so it's readable.

Is what you're suggesting much more common in code already? That would be a good reason to change.

I'm not sure what's more common, but indeed the problem for me is that it doesn't stand out. Especially for longer expressions such as

a + b*c*d*e**f

so we just disagree on

I think putting ** at the end of subexpressions without spaces is enough to distinguish it visually from *.

But yeah, I'm happy to be outvoted on this one. Just for the record!

And yeah, I think that's what we need is more opinions. Maybe we could add a limit on the number of chained operators in subexpressions without spaces, because I agree that b*c*d*e**f is not great style. But then maybe that's one of those cases where the last rule says - here, we can break a rule that has stopped making sense in this extreme situation.

lucascolley · 2024-08-11T09:33:27Z

+    * z)
+   ```
+   If there are multiple candidates, include the break at the first opportunity.
+9. The packages or subpackages from which mathematical functions and constants are used


I agree that this guidance is useful, however it does seem to be a bit of a digression from the main topic of the SPEC. Could it become a separate proposal, or something like an appendix?

mdhaber · 2024-08-11T15:20:29Z

@tupui I can take out the import guidance. Perhaps there can be a separate SPEC about that.

Are there other things here that we don't need other opinions about before changing?

tupui · 2024-08-11T16:40:40Z

On my side this can go in if you're both happy. Next step then to me would be to make another wide announcement and give some time for feedback. At that point I would also ping Charlie and Lukas for their inputs since in the end they are the ones who will probably have to put this in Ruff and Black.

mdhaber · 2024-08-11T17:27:02Z

Let me make the fixes I've indicated here before merge. But I would start with Charlie and Lukas to see if these are precise enough to be implementable before announcing.

tupui · 2024-08-11T17:32:51Z

Sounds good 👍

mdhaber · 2024-08-11T17:46:49Z

OK, I committed the inline comments and removed Rule 9 about imports. Let's see whether the Ruff/Black developers think these are close to being implementable.

tupui · 2024-08-11T19:07:10Z

Ok thanks 🙏 merging and will ping them on the real MR

Attempt to make SPEC 12 complete and unambiguous

f46a93e

mdhaber force-pushed the spec_12 branch from a5f5d20 to f46a93e Compare July 14, 2024 17:11

mdhaber commented Jul 14, 2024

View reviewed changes

Apply suggestions from code review

5f4b20a

Co-authored-by: Matt Haberland <mhaberla@calpoly.edu>

Improvements per self-review

34aa825

tupui reviewed Jul 15, 2024

View reviewed changes

mdhaber commented Jul 15, 2024

View reviewed changes

lucascolley reviewed Aug 11, 2024

View reviewed changes

mdhaber added 3 commits August 11, 2024 10:40

Apply suggestions from code review

63d45e6

Apply suggestions from code review

f2a96a6

Remove old rule 9

efa2ea8

tupui merged commit 8065ec6 into tupui:spec_12 Aug 11, 2024

mdhaber mentioned this pull request Aug 12, 2024

SPEC 12: Formatting mathematical expressions scientific-python/specs#326

Open

Conversation

mdhaber commented Jul 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdhaber left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdhaber commented Jul 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tupui left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdhaber commented Aug 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

mdhaber commented Jul 14, 2024 •

edited

Loading

mdhaber left a comment •

edited

Loading

mdhaber commented Jul 14, 2024 •

edited

Loading

mdhaber commented Aug 11, 2024 •

edited

Loading

mdhaber Aug 11, 2024 •

edited

Loading

mdhaber Aug 11, 2024 •

edited

Loading

mdhaber Aug 11, 2024 •

edited

Loading