Repository navigation
STY: group large int literals with PEP 515 underscores - #20326
Pierre-Sassoulas wants to merge 5 commits into
Conversation
Following pylint's bad-number-notation check with --suggest-int-underscore=y, add visual digit grouping to integer literals >= 1e6: groups of three for decimal (e.g. 2_446_354), groups of four for hex (e.g. 0xFFFF_FFFF). PEP 515 underscores are purely cosmetic — int values are byte-identical.
github-actions
Bot
added
io.votable
wcs
table
io.fits
config
time
io.ascii
nddata
units
coordinates
modeling
utils
wcs.wcsapi
uncertainty
labels
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
pllim
added
Extra CI
astrofrog
left a comment
astrofrog
left a comment
There was a problem hiding this comment.
I think this does improve readability, it might make sense to wait for another approval but I don't think all subpackage maintainers need to review
|
Thanks for splitting this out. I have to respectfully push back, though - I don't think this improves readability in most of the cases touched here, and I'd rather not adopt it as a codebase-wide rule. I'm simply not convinced it's a win. Where the grouping carries meaning, I agree it helps: The rule also seems to be "7+ digits" rather than any readability criterion, which produces inconsistent lines like assert_time_at(header_time_1d, 1, 2_450_003, 0.1 + 7 / 3600 / 24, ...)Why I realize others may disagree with me on the readability question itself. Since style rules like this affect everyone and generate churn across the whole codebase (37 files here), I think they warrant broader agreement than a single approval - at least a majority and ideally a supermajority of active maintainers - before being adopted and enforced by a linter. |
|
I was expecting to not be happy with the changes, but after looking at them I came around to the view that mostly they do improve readability. There may be a minority of cases that are worse (and maybe they can be excluded?), but overall I'm 👍 on this. I think that the limit at 7+ characters is a good one. |
taldcroft
left a comment
taldcroft
left a comment
There was a problem hiding this comment.
OK for time, table, io.ascii.
Runs of consecutive digits (123456789, 1234567890, 987654321, 1234567)
are read by their shape, and 2147483647 / 9223372036854775807 and their
negative counterparts are recognized on sight; grouping hides both.
pylint's bad-integer-notation now skips digit runs on its own, and the
integer limits go in its ignored-integers option:
ignored-integers=2147483647,2147483648,9223372036854775807,9223372036854775808
Hex masks such as 0xFFFF_FFFF stay grouped by 16-bit word.
Following review: - 0x4D3D3D3: grouping by 16-bit word breaks the visible D3D3D3 repetition. - test_header.py: the long integer is compared with its string form on the next line, so both should read the same. - seed=5301753 and ZBLANK = 1231212: an RNG seed and a blank sentinel, whose magnitude is irrelevant.
|
Thank you for the reviews! I changed pylint skips runs of consecutive digits (123456789, 1234567890, 987654321, 1234567) so meaningless number are not linted. I also added an option to ignore some integers. For astropy I used this config (DEFAULT_ZBLANK, ZBLANK, val_range): [FORMAT]
ignored-integers=2147483647,2147483648,4294967295,9223372036854775807,9223372036854775808On 2_450_003 vs 3600: the threshold is 10⁶, where '%g' switches to exponent notation and where a run of digits stops being countable at a glance (human brain sucks at handling more than ~= 5 things). So 3600 is considered short enough to read. There's also an option to change that, I can do things differentely in this PR if you want i.e. increase or decrease the threshold. Took into account what mcara said about some numbers being worse in this PR specifically but I guess this is going to have to be dealth with either inline disable or global disable via ignored-integers in pylint because it's hard to create a generic rule that is simple and works all the time. I'm not sure repetition always means one don't want to group. I'm not adding any linting in the CI here, as the checker is not released yet and getting refined with your help :). I can do that after a release if you want to enforce it. |
| assert_array_equal( | ||
| self.array["int"], [268435456, 2147483647, -268435456, 268435455, 123456789] | ||
| self.array["int"], | ||
| [268_435_456, 2147483647, -268_435_456, 268_435_455, 123456789], |
There was a problem hiding this comment.
Why is the underscore added inconsistently here?
There was a problem hiding this comment.
Following the review at #20326 (comment) I ignored some int. Maybe we shouldn't ignore them ? Or ignore the others too ?
There was a problem hiding this comment.
My concern is only this one line. I don't understand why the second and the last elements were excluded. Please clarify. Thanks.
There was a problem hiding this comment.
Basically mcara said it was worse to use _ on those numbers so I ignored 123456789 and 2147483647 and them only.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
@Pierre-Sassoulas , for this one line in test_vo.py, as one of the current io.votable maintainers, I hereby requests that you either underscore all the 5 values in the same list here, or don't do it at all. I don't care either way, but adding underscore to just 3 out of the 5 values really bothers me. Thanks for your understanding.
There was a problem hiding this comment.
My comment in #20326 (comment) was mostly referring to single rng seed values or other stuff without specific meaning or special constants such as pi or log base or Plank/Boltzmann constants that are ingrained in my brain from textbooks. I would prefer them to stay "uninterrupted" by underscores. Here, while 123456789 is one of those meaningless values, not underscoring it kind of interrupts the flow.
On the other hand, let's take 268435456. Where does this value come from? I am someone who never worked on that part of the package and I am not familiar with these constants. So I searched in VSCode all files that contain 268435456 and I found it comes from regression.bin.tabledata.truth.*.xml and regression.xml. Now, if we adopt these changes, would searching for 268_435_456 help me understand where the value comes from? So, my point, in general, is that we should probably leave this grouping to authors to do as they consider best. As for me - I find that this makes things only more difficult overall.
There was a problem hiding this comment.
The same applies to 922337203685477: this value appears in an XML file and is checked against in the unit tests. Being able to search for the literal and find every occurrence matters to me - that is the real "readability" benefit in my view. Making it easier to read off "nine hundred twenty-two trillion three hundred thirty-seven billion two hundred three million six hundred eighty-five thousand four hundred seventy-seven" doesn't actually help me understand the code. That said, I won't block this change if others find the three-digit grouping genuinely useful.
Description
This is a split of #19597 to change only using pep515 on int (more consensual than other proposals). floats can have a lot of reason to not be normalized, whereas no other alternative to make big int easier to read than pep 515 exists.
I evolved the pylint checker accordingly with three messages: bad-float-notation (disabled by default, scientific/engineering/pep615 allowed by default), bad-integer-notation and bad-float-precision instead of the initial bad-float-notation raising for bad-float-precision and with an option to parse int too.
Let me know if you'd rather I split this by subpackages.