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

Allow whitespace alignment of tables of numbers? #12367

Closed
endolith opened this issue Jun 14, 2020 · 7 comments · Fixed by #21851
Closed

Allow whitespace alignment of tables of numbers? #12367

endolith opened this issue Jun 14, 2020 · 7 comments · Fixed by #21851
Labels
maintenance Items related to regular maintenance tasks
Milestone

Comments

@endolith
Copy link
Member

For example, np.arange(0, 120).reshape(12, 10) is formatted by NumPy as

array([[  0,   1,   2,   3,   4,   5,   6,   7,   8,   9],
       [ 10,  11,  12,  13,  14,  15,  16,  17,  18,  19],
       [ 20,  21,  22,  23,  24,  25,  26,  27,  28,  29],
       [ 30,  31,  32,  33,  34,  35,  36,  37,  38,  39],
       [ 40,  41,  42,  43,  44,  45,  46,  47,  48,  49],
       [ 50,  51,  52,  53,  54,  55,  56,  57,  58,  59],
       [ 60,  61,  62,  63,  64,  65,  66,  67,  68,  69],
       [ 70,  71,  72,  73,  74,  75,  76,  77,  78,  79],
       [ 80,  81,  82,  83,  84,  85,  86,  87,  88,  89],
       [ 90,  91,  92,  93,  94,  95,  96,  97,  98,  99],
       [100, 101, 102, 103, 104, 105, 106, 107, 108, 109],
       [110, 111, 112, 113, 114, 115, 116, 117, 118, 119]])

but the linters used in unit testing reject tables of numbers like this because of rules like these:

  • E201 whitespace after '['
  • E202 whitespace before ']'
  • E221 multiple spaces before operator
  • E222 multiple spaces after operator
  • E241 multiple spaces after ','

and require changing to the less readable

array([[0, 1, 2, 3, 4, 5, 6, 7, 8, 9],
       [10, 11, 12, 13, 14, 15, 16, 17, 18, 19],
       [20, 21, 22, 23, 24, 25, 26, 27, 28, 29],
       [30, 31, 32, 33, 34, 35, 36, 37, 38, 39],
       [40, 41, 42, 43, 44, 45, 46, 47, 48, 49],
       [50, 51, 52, 53, 54, 55, 56, 57, 58, 59],
       [60, 61, 62, 63, 64, 65, 66, 67, 68, 69],
       [70, 71, 72, 73, 74, 75, 76, 77, 78, 79],
       [80, 81, 82, 83, 84, 85, 86, 87, 88, 89],
       [90, 91, 92, 93, 94, 95, 96, 97, 98, 99],
       [100, 101, 102, 103, 104, 105, 106, 107, 108, 109],
       [110, 111, 112, 113, 114, 115, 116, 117, 118, 119]])

I've listed a bunch of other examples here: PyCQA/pycodestyle#289 (comment)

I think this is a misapplication of PEP8, and this type of formatting should be allowed, but turning off these error codes completely would be bad, since they are much broader than this, and are correct for one-line lists, etc.

Ideally the rules for tables of data would be changed in PEP8 and pycodestyle, so it trickles down to everything that depends on them, but my attempts at requesting changes to pycodestyle and PEP8 were rejected: PyCQA/pycodestyle#289 python/peps#1428 If you know of a better place to lobby for this, we could try that.

Otherwise, it seems all we could do is implement plugins to flake8 to prevent this from happening? Maybe there are other workarounds.

See also spyder-ide/spyder#9955 and this PR failure and this PR failure.

@WarrenWeckesser
Copy link
Member

I also made a request to pycodestyle in PyCQA/pycodestyle#924, before I saw @endolith's earlier issue and comments.

I agree that we should be able to create aligned arrays of data without our style checker complaining about whitespace issues. I would have been OK (if not completely happy) with appending # noqa at the end of each line that is part of such an array, but that doesn't disable pycodestyle's whitespace checker.

With flake8, specific rules can be turned off, so one could use, say, # noqa: E201, E241. I haven't looked into what is involved in creating a flake8 plugin--maybe a plugin could be written that would require even less inline markup for this. Even without a plugin, I can support switching to flake8 in our CI style checks so we can use the # noqa markup with arrays of numbers.

@larsoner
Copy link
Member

I can support switching to flake8 in our CI style checks so we can use the # noqa markup with arrays of numbers.

I have made use of flake8 in other repos for years and indeed the line-based # noqa support has been nice in cases where exceptions should be made. Maybe we should go with that for now until a flake8-array-spacing or similar plugin can be made by someone with a motivation to do so (it would be useful!).

@mdhaber
Copy link
Contributor

mdhaber commented Jul 8, 2020

gh-12367 is wondering what to do about this sort of thing. Remove the offending whitespace for now?

@larsoner
Copy link
Member

larsoner commented Jul 9, 2020

@mdhaber I think you accidentally linked back to this issue instead of some other PR (?). In any case, for now I think remove it or or # noqa the lines of interest

@larsoner
Copy link
Member

larsoner commented Jul 9, 2020

I think in theory we can implement this without too much work or complexity in a separate flake8-array-spacing plugin. The only caveat is that we basically have to do recast the errors as a new type, so for example this code:

_ok = [ -3  + 4j ,  -10 +  20j ]
...

Will then produce when calling:

$ flake8 --ignore E201,E202,E203,E221,E222,E241 --select A --array-spacing flake8_array_spacing
flake8_array_spacing/__init__.py:1:8: A201 whitespace after '['
flake8_array_spacing/__init__.py:1:11: A221 multiple spaces before operator
flake8_array_spacing/__init__.py:1:17: A203 whitespace before ','
flake8_array_spacing/__init__.py:1:19: A241 multiple spaces after ','
flake8_array_spacing/__init__.py:1:26: A222 multiple spaces after operator
flake8_array_spacing/__init__.py:1:31: A202 whitespace before ']'

These new A errors, which are currently the same as the E variants, were made by a tiny plugin (see https://gitlab.com/pycqa/flake8/-/issues/673). All that's needed to make it properly ignore these for SciPy would be to make it not emit the errors when they occur as part of a list (of list) of numerical, which I think we can do with a regex.

So if people are okay with:

  1. Switching to flake8
  2. Installing a new flake8-array-spacing plugin
  3. Adding --ignore ... --select to the flake8 call (though really I would just do this in a setup.cfg instead)
  4. Living with knowing that "A" flavor errors are really just "E" flavors with array-exceptions

Then I think we can make this work.

@larsoner
Copy link
Member

larsoner commented Jul 9, 2020

One possible option up at #12516

@rgommers rgommers added the Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org label Feb 22, 2022
@lucascolley
Copy link
Member

these are now allowed by the current lint suite

@lucascolley lucascolley added maintenance Items related to regular maintenance tasks and removed Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org labels Nov 9, 2024
@j-bowhay j-bowhay added this to the 1.15.0 milestone Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Items related to regular maintenance tasks
Projects
None yet
7 participants