Notes from dev-telecon: CODEOWNERS, codecov, black

[Moritz Guenther]

Hi,

we don't usually send around notes after each dev-telecon, since they can be found in the same link as the agenda, that I emailed out just a few days ago: https://docs.google.com/document/d/15JSFh3OMF9Iz6ov3q_xxGO_BL8hRnuse4IMUrqEIvcg/edit?usp=sharing

However, this time, we discussed a few items that might have an impact on how PRs are written and reviewed in the future (see link above for details):

- In Github, you can subscribe to the full astropy repro and get an email notification for everything (which is a lot for core astropy) or to individual issues/PRs. Some people would prefer notification by sub-package (e.g. get an email for every PR that changes a file in astropy/io/ascii). This can be done with a CODEOWNERS file. William J will set that up for one case and if you want to opt-in into this yourself, you can simply do a PR to that file and add yourself for any subpackage you like. Note that this file *could* also be used to require PR reviews by specific people. WE WILL NOT DO THAT. Any maintainer can continue to merge any PR in astropy, we only use the "subscribe to notification" functionality (no PR yet).

- Codecov CI checks are sometimes spurious, e.g. a test is added, and that seems to decrease coverage and fail the CI. We agreed to lower the threshold (https://github.com/astropy/astropy/pull/13240 ) a bit and see if that helps.

- Formatting code can be contentious. Tom A is looking into using Black to autoformat and code touched in a PR using a pre-commit hook. Some people like it, some don't, but it would be useful to get some real live experience if that simplifies the experience of new or regular contributors or not. Ideally, we could activate this for some subpackage (e.g. io.ascii) and see how it goes before we decide on the next steps (no PR yet).

If you have any strong feelings or other good ides on these issues, this mailing list is a good place to discuss it; you might also try to attend the next dev telecon (2022-06-09, 14:00 UTC) and put it on the agenda for tht meeting again. (Same link as above).

Yours,

Moritz

[Tim Jenness]

On May 10, 2022, at 11:56, Moritz Guenther <moritz....@gmx.de> wrote:

- Formatting code can be contentious. Tom A is looking into using Black to autoformat and code touched in a PR using a pre-commit hook. Some people like it, some don't, but it would be useful to get some real live experience if that simplifies the experience of new or regular contributors or not. Ideally, we could activate this for some subpackage (e.g. io.ascii) and see how it goes before we decide on the next steps (no PR yet).

At Rubin we are using black more and more (along with isort) and it’s made all the code formatting discussions disappear. For those of us using it I haven’t heard any complaints. It frees you up from having to worry about formatting as you throw code at your editor and pre-commit ensures that everything is going to be committed okay (we do still have a GitHub action that checks formatting for those people who are not yet using pre-commit). pre-commit even has the advantage that it will install exactly the versions that your repo requires.

— 

Tim Jenness

[Pey Lian Lim]

Re: CODEOWNERS -- I still think only people who are interested to *review* the PRs should sign up, but we won't require approval from those people for merge (the branch protection part). If you are passing by and want to know what is going on, you can easily do a search on GitHub by labels on your own via existing GitHub UI or API, e.g., https://github.com/astropy/astropy/pulls?q=is%3Apr+is%3Aopen+label%3Aio.ascii (all currently open PRs for io.ascii).

[Aldcroft, Tom]

So maybe it is time to open the floodgates for discussion... :-)

I brought this up at the dev meeting, and started by saying that even 2-3 years ago I looked at black-formatted code and didn't like it. But somewhere along the way it clicked and I realized that the style decisions that black has made are appealing to me. The general move to not-as-much on a single line is a good thing and improves readability. The lack of configurability also appeals to me so we don't need a separate big thread to decide on the defaults.

Numpy is having the same discussion (which is what got me inspired). There is a proof of concept for numpy. See https://mail.python.org/archives/list/numpy-di...@python.org/thread/WZTS3XZKKM3MKRPZ5XD6SHSREYLTQ3DC/ and 

https://github.com/numpy/numpy/pull/21449.

On the numpy thread there was not actually that much dissent. I get the feeling a lot of projects are going this way.

William Jamieson has volunteered to look into implementing something for astropy, possibly just doing auto-formatting on a per-subpackage basis. The key point is that this should happen locally when a developer makes a commit, so it is intended to be automatic and require no user effort. This will just be a proof-of-concept to make it easier to see how it might work in practice.

If anyone would like to share on the developer experience, that would be good. In any case, this is your chance to weigh in with thoughts pro or con!

- Tom

--
You received this message because you are subscribed to the Google Groups "astropy-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to astropy-dev...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/astropy-dev/B4FD10AC-6CB5-4B9A-AC2E-C32A96E50685%40gmail.com.

[Tim Jenness]

On May 10, 2022, at 15:01, Aldcroft, Tom <tald...@gmail.com> wrote:

So maybe it is time to open the floodgates for discussion... :-)

I brought this up at the dev meeting, and started by saying that even 2-3 years ago I looked at black-formatted code and didn't like it. But somewhere along the way it clicked and I realized that the style decisions that black has made are appealing to me. The general move to not-as-much on a single line is a good thing and improves readability. The lack of configurability also appeals to me so we don't need a separate big thread to decide on the defaults.

Yes. I was also skeptical a while back but now that I’ve done it I think it’s great. I don’t care any more.

At Rubin we did make one configuration change and that was to allow 110 character line-lengths since that is what our project likes.

Please also consider isort.

Numpy is having the same discussion (which is what got me inspired). There is a proof of concept for numpy. See https://mail.python.org/archives/list/numpy-di...@python.org/thread/WZTS3XZKKM3MKRPZ5XD6SHSREYLTQ3DC/ and 

https://github.com/numpy/numpy/pull/21449.

On the numpy thread there was not actually that much dissent. I get the feeling a lot of projects are going this way.

William Jamieson has volunteered to look into implementing something for astropy, possibly just doing auto-formatting on a per-subpackage basis. The key point is that this should happen locally when a developer makes a commit, so it is intended to be automatic and require no user effort. This will just be a proof-of-concept to make it easier to see how it might work in practice.

If anyone would like to share on the developer experience, that would be good. In any case, this is your chance to weigh in with thoughts pro or con!

I would caution against trying to mix and match black and no-black within a single git repo.  That way lies lots of confusion and I’m not really sure what we gain from doing that unless there are some people who are completely against the whole idea. It will make it harder to configure editors if people want to run black automatically. I have no idea if pre-commit can be told to only look for specific subdirectories when running pre-commit hooks.

The actual configuration change is only a few lines. For example

https://github.com/lsst/daf_butler/blob/main/pyproject.toml#L43-L49

and https://github.com/lsst/daf_butler/blob/main/.pre-commit-config.yaml

and we have a GitHub action to check: https://github.com/lsst/daf_butler/blob/main/.github/workflows/formatting.yaml

— 

Tim Jenness

[Marten van Kerkwijk]

Hi All,

My feeling is that black formats too much like a C programmer would -
especially with closing braces on their own line - and aims too much at
minimizing lines changed rather than optimizing for readability; I don't
feel it goes by the spirit of PEP8 even if it is formally compatible.

But obviously readability is subjective; I see the huge advantage of
just not having to discuss; and black seems to be the only option out
there. So, while I'm not really in favour, it is definitely not a big
enough deal to object beyond this e-mail!

All the best,

Marten

[William Jamieson]

Hi all,

I have opened a draft PR: https://github.com/astropy/astropy/pull/13253 for this (note that for now, all python code in astropy is excluded from black so that subpackages can opt-in). Feel free to add/move your comments/suggestions directly to this PR.

Cheers,

William Jamieson

[Thomas Robitaille]

Hi all,

I have no strong opinions about black anymore - it's not necessarily always how I would write code but on the other hand we would essentially never have to worry about going around fixing PEP8 errors and so on. It also looks like black is now maintained by the PSF. So I'm happy to go along if other maintainers prefer this.

One thing we should be clear about perhaps is how we are going to make a decision about this, and what to do about coordinated and infrastructure packages. I think we might want to leave it up to the maintainers of each coordinated package to decide what to do for their packages, and same for infrastructure packages (basically whoever has to work with the code most should decide). For the core package, I think this should be up to the core package maintainers, and if we reach the point where all but one or two core maintainers are in favor, we could go ahead but potentially exclude the sub-packages of those maintainers from ones where black would be used (because it is more important to not alienate maintainers than to have consistent code style).

Note that one thing we could do if we wanted aside from applying formatting just to changed code in PRs would be to format all files that aren't touched by any PR (this is easy to script) and do it regularly so that over time all the code base would end up being re-formatted with no impact on open PRs.

Cheers,

Tom

To view this discussion on the web visit https://groups.google.com/d/msgid/astropy-dev/CAMtEP6z6RsUvbL5G_e72tne-5tqjZAs6DO%3D-sygFGriyMAHO2A%40mail.gmail.com.

[Simon Conseil]

Hi all,

I'm also +1 for using black. Even if I don't like its style I think
automatic formatting is a great way to solve discussions about
formatting and have a clear and consistent style across the package.

I have been using yapf which offers more configuration, allowing to
have a style closer to what I like, but in the end the style itself
doesn't really matter (and as pointed in the Numpy discussion yapf is
not actively maintained).

However applying it only on some subpackages is maybe not a great idea,
it may lead to some confusion and we may see PRs formatting the code
where it shouldn't. It's OK for a test but would be better to use black
everywhere if we reach an agreement to use it.

And last controversial point is the line length... the current setting
of 100 is too much for me. Numpy plans to keep their current limit at
79 characters. Black's recommendation is 88 which seems a good
compromise to me (and I'm using that in some of my projects). The
current value of 100 was chosen to remove most warnings with flake8
(https://github.com/astropy/astropy/issues/9574) but that does not mean
it's the best value if we go with the automatic formatting :).
To compare with other scientific python projects, scikit-learn and
pandas use black with the 88 default value.

Cheers
Simon

> Tim Jenness
> --
> You received this message because you are subscribed to the Google
> Groups "astropy-dev" group.
> To unsubscribe from this group and stop receiving emails from it,
> send an email to astropy-dev...@googlegroups.com.
> To view this discussion on the web visit

> https://groups.google.com/d/msgid/astropy-dev/6D07FEDD-2E9E-4457-A344-E5418D727F43%40gmail.com
> .

[Aldcroft, Tom]

About line length, I was one of the original "give me 100 characters" people (as evidenced in time, io.ascii, table, but I've evolved and I'm happy with the black default recommendation of 88. From what I have seen the way black ends up splitting things the 88 characters works pretty well. I also have switched to using a laptop 100% of the time and so 88 actually helps me out a bit for fitting two files side by side with a reasonable font size.

I'm also in favor of going all-in on black for all of astropy core. Thus far there has been agreement ranging from tepid to enthusiastic, with only one mild disagreement from Marten (but he explicitly said he would agree if others are in favor). So maybe we don't need to worry so much about per-subpackage preferences if no maintainers actually strongly disagree with black formatting. 

The point about open PR's is a good one, but I'm not quite sure how well Tom R's proposal would work in practice. We've had some "affects-every-file" changes in the past (e.g. Python 3-only), and we managed.

I agree with Tom R's recommendations about coordinated and infrastructure packages.

- Tom

To view this discussion on the web visit https://groups.google.com/d/msgid/astropy-dev/49aa36f4b995302f2b814f1502d2459046497262.camel%40sconseil.fr.

[Pey Lian Lim]

I said it at the telecon and I am going to also say it here: Please leave astropy.io.fits out of this. 

[William Jamieson]

Hi all,

Note, the 100 character limit can be changed if there is broad agreement (I have no strong feels on the subject). It is currently set there to agree with both the existing flake8 and isort (maybe others?) configurations.

[Aldcroft, Tom]

On Thu, May 12, 2022 at 3:26 PM Pey Lian Lim <p3y...@gmail.com> wrote:

I said it at the telecon and I am going to also say it here: Please leave astropy.io.fits out of this. 

Hi Pey Lian,

Your opinions are respected and valued, so I wonder if you can elaborate a bit on keeping io.fits out of black formatting.

Thanks,

Tom

[Homeier, Derek]

On 12 May 2022, at 8:02 pm, Aldcroft, Tom <tald...@gmail.com> wrote:

About line length, I was one of the original "give me 100 characters" people (as evidenced in time, io.ascii, table, but I've evolved and I'm happy with the black default recommendation of 88. From what I have seen the way black ends up splitting things the 88 characters works pretty well. I also have switched to using a laptop 100% of the time and so 88 actually helps me out a bit for fitting two files side by side with a reasonable font size.

So far Maarten’s response (see below_ has also pretty much summarised my thoughts about this best,

but I realise I did take it more or less for granted that, if the line length is configurable, we would stick to

the current 100 characters. Having also worked almost exclusively on 13-14” laptop screens, the width

argument does resonate (in fact it was only the decision to adopt 100 for Astropy that pushed me to

changing my default editor width from 80 for probably the first time since using a VT-100).

On the other extreme Tim even mentioned using a default limit of 110 at Rubin.

The main points I see in favour of 100 are

1. This has been upped from 79 fairly recently, and I think has and still is being probed to be adopted

by many coordinated and affiliated packages. Changing it back now, or rather changing it to yet a

third value, seems bound to create much unnecessary confusion.

2. I think it makes quite a significant difference in (not) hitting the limits that require multi-line splitting of

statements (which I guess was a main contributor to the previous flake8 conflicts).

This seems all the more crucial in combination with black, when it would often make the difference between

a 1-line and an 8-line function declaration or so.

That said, #1 might not be so critical since a) a lot of the existing code still adheres to the 79-char limit,

and b) black would take care of the reformatting (and a lot of code would look completely different) anyway.

Re. #2 I probably could be convinced, if 88 has really been found to be optimal by the black developers

themselves and prior adopters from scientific packages (perhaps more relevant, as the typical function

signatures, default parameters, [type annotations?] may be more applicable to our codebase).

On 11 May 2022, at 2:55 am, Marten van Kerkwijk <mh...@astro.utoronto.ca> wrote:

My feeling is that black formats too much like a C programmer would -
especially with closing braces on their own line - and aims too much at
minimizing lines changed rather than optimizing for readability; I don't
feel it goes by the spirit of PEP8 even if it is formally compatible.

But obviously readability is subjective; I see the huge advantage of
just not having to discuss; and black seems to be the only option out
there. So, while I'm not really in favour, it is definitely not a big
enough deal to object beyond this e-mail!

So on the whole I still second this, although looking at (two alternatives of) actual applications to code

https://github.com/WilliamJamieson/astropy/compare/black-config..black-config-alternate-multiline

I also find most of my worst preconceptions about the effects of blackening confirmed, and feel

Erik’s comments on the PR https://github.com/astropy/astropy/pull/13253#issuecomment-1125393938

still resonating a lot.

Best

Derek

[Aldcroft, Tom]

I went looking into the decision process of other large community projects that have adopted black. This includes scikit-learn, requests, django, numpy (leaning) and others mentioned at https://github.com/psf/black#used-by. There is no guarantee that those other large projects have chosen wisely, but looking at "prior art" might be a good idea.

A very nice reference to start with is the Django enhancement proposal #8 to adopt black formatting: I think this is a balanced review of the pro's and con's which explicitly recognizes that in a large project there will never be agreement on code style. This can cause friction in the code contribution process. The question then comes down to whether it is useful as a community open-source project to have an automated and neutral arbiter of style. The Django DEP argues that on the whole this lowers the bar for contributions by newcomers and makes code writing easier and code review easier. For skeptics, there are a few testimonials of developers that initially objected to the black style but quickly adapted and learned to at least accept it. [I can't say if there was a bias in choosing quotes, of course].

For scikit-learn the public discussion I found is here: https://github.com/scikit-learn/scikit-learn/issues/11336. They reference a developers telecon, where presumably most of the discussion occurred.

- Tom

[Pey Lian Lim]

Re: FITS -- We don't have the resources to debug it if some formatting fix accidentally break something subtle. It is old code and complicated.

[Marten van Kerkwijk]

Hi All,

I agree that if we go ahead, ideally we do it for the whole core package.

I'm also in favour of using the default 88 character count (or 80);
100 is just a bit too much (indeed, for having two files open next to
each other on a laptop!).

But Tom R's point about PRs gives me a bit of thought: once we have done
this, it will be pain to compare files over different versions, to trace
why/how changes were made, etc. Though it depends, I guess, on what
fraction of a typical file is impacted. This may be a particular issue
for older code like io.fits, where the changes may be larger, somewhat
going along with what Pey-Lian just wrote.

Anyway, maybe the discussion should move more towards whether it is
possible to migrate to this in as painless a way as possible.

All the best,

Marten

[Tim Jenness]

> On May 13, 2022, at 12:20, Pey Lian Lim <p3y...@gmail.com> wrote:
>
> Re: FITS -- We don't have the resources to debug it if some formatting fix accidentally break something subtle. It is old code and complicated.
>

Black guarantees that the code that you end up with generates exactly the same bytecode as what you started with. By design it is impossible for black to break something.

—
Tim Jenness

[Simon Conseil]

Indeed, I don't think there will any issue with reformatting the
io.fits code. I just did it locally and ran the tests, everything is
fine. io.fits is certainly complicated but it's because of much deeper
issues than formatting (at first because of the FITS standard
itself!)).

Simon

[Simon Conseil]

About tracing changes, just a note that it's possible to exclude some
commits for git-blame, e.g. for scikit-learn:
https://github.com/scikit-learn/scikit-learn/blob/main/.git-blame-ignore-revs

Also I don't think io.fits is so different from other sub-packages ;).

Simon

[Pey Lian Lim]

My sentiment is somewhere between Marten's initial reaction and Erik's, but looks like we're the minority here...

[Stuart Mumford]

I will add that I am generally -1 on this, both from a personal
preference and also a practical standpoint. We seem to be getting on
perfectly fine without black most of the time, so I am not really sure
it's worth the disruption?


Stuart

[Aldcroft, Tom]

Fair point about effort vs. benefit. One of the benefits that has been cited by projects and individuals adopting black is freedom from having to consider PEP8 and think about style standards while writing code. This applies in particular to less-experienced developers. Most of the code that I see from my team members in the CXC is very much not PEP8-compliant and would require a lot of feedback (or an automated tool) to meet astropy standards.

Many of the more experienced people on this list most likely code in PEP8-compliant ways instinctively or apply auto-formatting in the editor. So "getting on perfectly fine" might not reflect the experience of everyone.

It might seem counterintuitive, but by telling contributors to run an additional one-time step to `pre-commit install` or `black .`, then we may be lowering the entry bar for contributing to astropy.

- Tom

 

Stuart

--
You received this message because you are subscribed to the Google Groups "astropy-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to astropy-dev...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/astropy-dev/d351db37-ec06-4d66-2378-6e2e8c0876f7%40cadair.com.

[Eero Vaher]

Code formatted by Black tends to take up lots of vertical space, which is not quite to my liking, but I do recognize that it makes Git diffs less cluttered. So I would say we would be slightly trading off code readability for improved diff readability. I am in favor of applying Black with default settings (including 88 character line length) to all of astropy.

We already have more than two hundred commits in astropy main (a3c27114b964dd287f078c8d14cd10574ae4fba2) that explicitly mention PEP 8. That number is not going to get any smaller, but with Black it might stop growing.

```shell

$git log --grep="PEP\s*8" --pretty=oneline | wc -l
229

```

Eero Vaher

[Aldcroft, Tom]

On Tue, May 17, 2022 at 9:09 AM Eero Vaher <eero....@gmail.com> wrote:

Code formatted by Black tends to take up lots of vertical space, which is not quite to my liking, but I do recognize that it makes Git diffs less cluttered. So I would say we would be slightly trading off code readability for improved diff readability. I am in favor of applying Black with default settings (including 88 character line length) to all of astropy.

To this point I've purposely avoided making comments about code readability because that is quite subjective and personal. For me, I find the black style to be easier to read, even in parts of the code that I wrote myself. I find that typical multi-line argument lists in function definitions are difficult to read and putting one per line allows seeing the default for a particular arg more quickly. If we ever include type annotations this will be even more important. Overall the blocking and organization of multi-line statements fits my brain. Your mileage may vary.

There are also many examples that I believe most people would agree are more readable, e.g.:

We do have a few data points from skeptics that testified that they quickly got used to the black style and stopped noticing it.

 

We already have more than two hundred commits in astropy main (a3c27114b964dd287f078c8d14cd10574ae4fba2) that explicitly mention PEP 8. That number is not going to get any smaller, but with Black it might stop growing.

```shell

$git log --grep="PEP\s*8" --pretty=oneline | wc -l
229

```

 

And that doesn't include all the ones that were fixed in an amended commit.

- Tom

Eero Vaher

--
You received this message because you are subscribed to the Google Groups "astropy-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to astropy-dev...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/astropy-dev/01608250-1b39-4554-a4dd-b7db34c0f082n%40googlegroups.com.

[Homeier, Derek]

On 17 May 2022, at 2:25 pm, Aldcroft, Tom <tald...@gmail.com> wrote:

On Tue, May 17, 2022 at 6:50 AM Stuart Mumford <stu...@cadair.com> wrote:

I will add that I am generally -1 on this, both from a personal 
preference and also a practical standpoint. We seem to be getting on 
perfectly fine without black most of the time, so I am not really sure 
it's worth the disruption?

Fair point about effort vs. benefit. One of the benefits that has been cited by projects and individuals adopting black is freedom from having to consider PEP8 and think about style standards while writing code. This applies in particular to less-experienced developers. Most of the code that I see from my team members in the CXC is very much not PEP8-compliant and would require a lot of feedback (or an automated tool) to meet astropy standards.

Many of the more experienced people on this list most likely code in PEP8-compliant ways instinctively or apply auto-formatting in the editor. So "getting on perfectly fine" might not reflect the experience of everyone.

It might seem counterintuitive, but by telling contributors to run an additional one-time step to `pre-commit install` or `black .`, then we may be lowering the entry bar for contributing to astropy.

This reflects some thoughts I had while reading your link to the Django experiences and DEP.

That contains a good deal of useful information, and I would consider it as fairly objective,

but am hesitant to call it a “neutral” review of the situation – I think the authors are/have been

quite clearly supporting the adoption of black, and the quotes from the “converted” or now accepting

developers sound quite anecdotal and picked to demonstrate how even “known sceptics” came

to support or tolerate black. That is of course impossible to confirm without knowing more ot the

underlying discussion.

But more related to the above point, the first stated benefit was something I could relate to very little –

about spending a lot of time thinking and making small decisions about code formatting.

Admittedly I have not really tried the alternative, to code with black, but I hardly ever had the feeling

that formatting decisions consumed much of my time compared to, say, implementation details,

API decisions etc. If anything, using flycheck in the editor has probably taken away most of that

burden (plus providing extra checks on undefined/unused variables etc.)

So that might confirm your comment above that it will offer greater benefits to new/less experienced

contributors, but still leaves open how to evaluate the net benefit or cost to the entire developer base.

I am also wondering how applicable experiences from a project like Django are to more science-code

heavy projects; so the scikit examples may indeed be more helpful. Definitely

On 13 May 2022, at 10:37 pm, Simon Conseil <si...@sconseil.fr> wrote:

About tracing changes, just a note that it's possible to exclude some
commits for git-blame, e.g. for scikit-learn:
https://github.com/scikit-learn/scikit-learn/blob/main/.git-blame-ignore-revs

is good to know, since I found the Django reference to "View blame prior to this change” extremely

unsatisfying – I find changes generally much harder to trace back as soon as getting to the next

point in commit history, and if, after black application, this means almost always starting at level

–2, that would be a serious disadvantage IMO.

I was also wondering if it is feasible, to mediate this at least for command line diffs etc., to create

reference branches of all important milestones with black applied to them as comparison targets?

How to proceed towards a decision on this? From the discussion here I am interpreting

Tom A., William, Tim, Simon, Eero

to be in various degrees of support for switching to black,

Tom R., Moritz (?), [Abhijeet]

having been mostly neutral (I did not find any records of comments either way from them from the telecon).

Marten, Pey-Lian, Derek, Stuart, Erik

being sceptical or opposed in various (increasing left-to-right ;-) degrees.

Also there still are implementation questions open regarding application to existing code in all

or selected subpackages, line-length settings etc.

Cheers,

Derek

[Adrian Price-Whelan]

For what it's worth, I used to be strongly against auto-formatting (for personal stylistic preferences). But now I'm pretty neutral and use it occasionally, so I would be fine either way.

best,

Adrian

--
You received this message because you are subscribed to the Google Groups "astropy-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to astropy-dev...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/astropy-dev/35DBAE98-75ED-43CA-9B69-D27E4D03128A%40gwdg.de.

--

Adrian M. Price-Whelan (he / him)

Associate Research Scientist 

Center for Computational Astrophysics

Flatiron Institute

http://adrn.github.io

[Larry Bradley]

For what it's worth, I'm -1 to applying black to astropy, mainly because of personal style preferences -- it's not how I write code and generally I find it less readable.  I don't intend to apply it to photutils.

Cheers,

Larry

To view this discussion on the web visit https://groups.google.com/d/msgid/astropy-dev/CAEUL7mHT51tcFz7JCdtJfuYu-4ABC6AAQdqq3gO0Bnn9Rh6Gmg%40mail.gmail.com.

[Aldcroft, Tom]

From the GitHub issue, Nathaniel is +1 on black, and Karl Gordon (self-described "minor player") is +1, and we can add Adrian as neutral and Larry as -1.

Karl's testimonial is worth repeating here "It has been great for me to avoid having to deal with PEP8 issues and has been great for new contributors to the beast as learning how to write well formatted python code is not trivial."

I posted to the Django developers mailing list with questions about their experience. This is awaiting moderation (as a first-time poster). I have also applied to join the scikit-learn mailing list, which is pending. So I think we should at least wait for feedback from those projects that have gone down this road.

- Tom

On Tue, May 17, 2022 at 10:06 AM Adrian Price-Whelan <adri...@gmail.com> wrote:

To view this discussion on the web visit https://groups.google.com/d/msgid/astropy-dev/CAEUL7mHT51tcFz7JCdtJfuYu-4ABC6AAQdqq3gO0Bnn9Rh6Gmg%40mail.gmail.com.

[Adam Ginsburg]

I'm listening to this conversation from the sidelines and, at the moment, I'm pretty firmly neutral.  There are good arguments both ways.  I like making onboarding new users easier, though I'm not perfectly convinced yet that this change would achieve that.  I'm very concerned about what a broad-strokes code style change does to blamability / history.  My initial reaction was basically the same as Stuart's.

I would caution against interpreting those contributing to this conversation as a voting quorum. 

To view this discussion on the web visit https://groups.google.com/d/msgid/astropy-dev/CAMtEP6zQ0YqcFsTdMK0aviYYnfwiENYwpwZ9t8uF__FqS28nMA%40mail.gmail.com.

--

Adam Ginsburg
Assistant Professor, Department of Astronomy

University of Florida, Gainesville

http://www.adamgginsburg.com/

[Aldcroft, Tom]

On Tue, May 17, 2022 at 10:35 AM Adam Ginsburg <adam.g....@gmail.com> wrote:

I'm listening to this conversation from the sidelines and, at the moment, I'm pretty firmly neutral.  There are good arguments both ways.  I like making onboarding new users easier, though I'm not perfectly convinced yet that this change would achieve that.  I'm very concerned about what a broad-strokes code style change does to blamability / history.  My initial reaction was basically the same as Stuart's.

I would caution against interpreting those contributing to this conversation as a voting quorum. 

On that point I was just about to say that, given the roughly equal division of opinions, this will almost certainly need to go for a formal vote following the process outlined in our governance model. Woohoo!

- Tom

 

To view this discussion on the web visit https://groups.google.com/d/msgid/astropy-dev/CAEBNSwYaJX_fg1VPknQ%2BOwEQ2Y%3DZLTM7ffkf7Ls%2B-TKe0pU7Fw%40mail.gmail.com.

[Tim Jenness]

On May 17, 2022, at 05:25, Aldcroft, Tom <tald...@gmail.com> wrote:

On Tue, May 17, 2022 at 6:50 AM Stuart Mumford <stu...@cadair.com> wrote:

I will add that I am generally -1 on this, both from a personal
preference and also a practical standpoint. We seem to be getting on
perfectly fine without black most of the time, so I am not really sure
it's worth the disruption?

Fair point about effort vs. benefit. One of the benefits that has been cited by projects and individuals adopting black is freedom from having to consider PEP8 and think about style standards while writing code. This applies in particular to less-experienced developers. Most of the code that I see from my team members in the CXC is very much not PEP8-compliant and would require a lot of feedback (or an automated tool) to meet astropy standards.

Many of the more experienced people on this list most likely code in PEP8-compliant ways instinctively or apply auto-formatting in the editor. So "getting on perfectly fine" might not reflect the experience of everyone.

As an experienced developer who codes in PEP8 instinctively at this point I am really happy with our switch to black at Rubin. I couldn’t see the point for a long while but now I know that I can write my almost PEP8 compliant code (especially when it comes to a line suddenly being longer than the limit) and know that I don’t have to spend the next few seconds sorting out where to wrap. I just run black (or wait for the pre-commit to trigger) and know that it will be fine.

Also, auto-formatting in an editor when black is not being used needs restraint. I’ve even had problems with astropy PRs where my editor will strip a trailing space from some other place in the file and I had to be careful to make sure I didn’t commit that when making my PR.

— 

Tim Jenness

[Tom Aldcroft]

On Tue, May 17, 2022 at 12:55 PM Tim Jenness <tim.j...@gmail.com> wrote:

On May 17, 2022, at 05:25, Aldcroft, Tom <tald...@gmail.com> wrote:

On Tue, May 17, 2022 at 6:50 AM Stuart Mumford <stu...@cadair.com> wrote:

I will add that I am generally -1 on this, both from a personal
preference and also a practical standpoint. We seem to be getting on
perfectly fine without black most of the time, so I am not really sure
it's worth the disruption?

Fair point about effort vs. benefit. One of the benefits that has been cited by projects and individuals adopting black is freedom from having to consider PEP8 and think about style standards while writing code. This applies in particular to less-experienced developers. Most of the code that I see from my team members in the CXC is very much not PEP8-compliant and would require a lot of feedback (or an automated tool) to meet astropy standards.

Many of the more experienced people on this list most likely code in PEP8-compliant ways instinctively or apply auto-formatting in the editor. So "getting on perfectly fine" might not reflect the experience of everyone.

As an experienced developer who codes in PEP8 instinctively at this point I am really happy with our switch to black at Rubin. I couldn’t see the point for a long while but now I know that I can write my almost PEP8 compliant code (especially when it comes to a line suddenly being longer than the limit) and know that I don’t have to spend the next few seconds sorting out where to wrap. I just run black (or wait for the pre-commit to trigger) and know that it will be fine.

This experience echoes mine. I fuss a lot (too much) about how the code looks and reads, and find myself commonly reformatting a statement, thinking about argument placement, moving a parenthesis, or the worst case of adding a new function to _all_ where it logically fits, finding it needs a wrap, and then actually (by hand) reformatting all the rest to keep things tidy.

So I'm one of those folks who just want to stop thinking about style because it wastes my time, while I am not passionate about the precise details of the style.

- Tom

 

Also, auto-formatting in an editor when black is not being used needs restraint. I’ve even had problems with astropy PRs where my editor will strip a trailing space from some other place in the file and I had to be careful to make sure I didn’t commit that when making my PR.

— 

Tim Jenness

--
You received this message because you are subscribed to the Google Groups "astropy-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to astropy-dev...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/astropy-dev/3AECBC0E-B0EF-495C-8657-A7E1AF7440C8%40gmail.com.

[Thomas Robitaille]

Hi all,

A few follow-up thoughts:

- It might be worth separating the question in a similar way to what the Django enhancement proposal did, which is (a) whether people are in favor of auto-formatters in general, and if so then (b) whether anyone has a better suggestion than black specifically?

- I would suggest not worrying for now about line lengths, as that is to some extent a detail that can be figured out if we do decide to go with black or another auto-formatter. If anything, it might make sense to assume for now we would just continue with the same line limit and then make the decision to change line lengths separately from the auto-formatting. Of course, we should make sure that decision is made before we actually implement any auto-formatting changes but it seems early to discuss it now.

- We are going to have to be careful about how this impacts backports to release branches - if we apply black to all the code in the core package in main but not in the release branches then backports are going to be much more complicated. If we do any significant reformatting (not just for PR diffs) then the best timing for that might be after feature freeze and before the next major release, because at that point there will be the least number of PRs open, the previous main release branch will no longer need backports, and the LTS version will already be 1 year old so the number of critical backports needed is going to be lower. In fact the ideal time would be after the 6.0 feature freeze when there are no longer any old active release branches, but that might be too long to wait.

- If there is agreement on using black or another auto-formatter, I think we should then have a dedicated call (or use one of the dev calls for that) to figure out some details (including timing and so on for release branches) before going ahead with implementing it or merging any PRs related to this.

Cheers,

Tom

--
You received this message because you are subscribed to the Google Groups "astropy-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to astropy-dev...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/astropy-dev/a804aaa0-750b-41b2-86a8-8e218988e229n%40googlegroups.com.

[Juan BC]

Its imposible to black to introduce new bugs because preserve the original abstract syntax tree. 

[Aldcroft, Tom]

tl;dr: I'd propose that we start planning a vote to accept or not accept the concept of auto-formatting with black. Implementation details (line length, timing) would be discussed only if the concept is approved.

I started thinking about how to even have such a vote. One idea is that anyone who wants to express an opinion one way or another can do so in a channel on the astropy discourse (as if they were candidates) prior to having the vote. This is then put to a vote of the astropy voting members. But I'm getting out of my lane since this is CoCo business.  :-) 

See below for context.

On Tue, May 17, 2022 at 4:31 PM Thomas Robitaille <thomas.r...@gmail.com> wrote:

Hi all,

A few follow-up thoughts:

- It might be worth separating the question in a similar way to what the Django enhancement proposal did, which is (a) whether people are in favor of auto-formatters in general, and if so then (b) whether anyone has a better suggestion than black specifically?

In the end I think this is going to come down to a formal Yes/No community vote because the issue is too divided to reach consensus by email or otherwise. I think the way the above question is structured is not really conducive to a vote because (a) and (b) are somewhat coupled. Some people might vote Yes to (a) only as long as the formatter is yapf and the format configuration is acceptable, but that would depend non-deterministically on a future discussion of the style.

This is again hitting the philosophy of black, namely simplifying decisions by reducing options. We can handle agreeing on one parameter (line length). Beyond that... :-)

To quote from DEP-0008 regarding adopting yapf and tuning it:

The point of adopting an automatic code formatter isn't to have our own Django astropy-flavored code formatting style. It's about making our Python code look as much as possible like what everyone else in the Python community writes. Code formatters maximize their usefulness by not being configurable.

Implied in this is that I'm not aware of any other full-featured code formatters in wide use besides yapf, black and autopep8. But autopep8 is timid and won't do things like introduce parentheses to fix the style.

 

- I would suggest not worrying for now about line lengths, as that is to some extent a detail that can be figured out if we do decide to go with black or another auto-formatter. If anything, it might make sense to assume for now we would just continue with the same line limit and then make the decision to change line lengths separately from the auto-formatting. Of course, we should make sure that decision is made before we actually implement any auto-formatting changes but it seems early to discuss it now.

Agreed.

 

- We are going to have to be careful about how this impacts backports to release branches - if we apply black to all the code in the core package in main but not in the release branches then backports are going to be much more complicated. If we do any significant reformatting (not just for PR diffs) then the best timing for that might be after feature freeze and before the next major release, because at that point there will be the least number of PRs open, the previous main release branch will no longer need backports, and the LTS version will already be 1 year old so the number of critical backports needed is going to be lower. In fact the ideal time would be after the 6.0 feature freeze when there are no longer any old active release branches, but that might be too long to wait.

Good points, and I'm sure we can find a reasonable solution. It seems like no matter what we have the option of waiting until 6.0, which is about a year from now?

 

- If there is agreement on using black or another auto-formatter, I think we should then have a dedicated call (or use one of the dev calls for that) to figure out some details (including timing and so on for release branches) before going ahead with implementing it or merging any PRs related to this.

Yes, we should be clear that the current PR's are really implementation demonstrations and not intended for merging without community and infrastructure team concurrence.

- Tom

 

To view this discussion on the web visit https://groups.google.com/d/msgid/astropy-dev/CAGMHX_3K_rG74KA-4bNwpOgD%3DJ_D8xn6tw%3DduDGmHYkPCYOSbQ%40mail.gmail.com.

[Marten van Kerkwijk]

Hi Tom, others,

I agree that we probably should set up something like a vote. I looked
a bit at the example put up for modelling --
https://github.com/astropy/astropy/pull/13254/files
-- and it seems clear to me there will need to be some exceptions: it
looks like black does particularly poorly for math (see below).

Note that I didn't find too much that to me was truly wrong, and I guess
exceptions for exceptional cases are OK. But this does mean one will
actually have to look at all those black PRs to explicitly exclude some
lines, which should weigh in any decision.

All the best,

Marten

Two examples where I think black destroyed formatting:

Existing:

numer = (((((((((((((((AA[15]*Z + AA[14])*Z + AA[13])*Z + AA[12])*Z + AA[11])*Z +
AA[10])*Z + AA[9])*Z + AA[8])*Z + AA[7])*Z + AA[6])*Z +
AA[5])*Z + AA[4])*Z+AA[3])*Z + AA[2])*Z + AA[1])*Z + AA[0])
denom = (((((((ZZ + bb[14])*ZZ + bb[12])*ZZ + bb[10])*ZZ+bb[8])*ZZ + bb[6])*ZZ +
bb[4])*ZZ + bb[2])*ZZ + bb[0]

Black's version (note inconsistency in middle; but really what gets me
is the closing parentheses on their own line; just useless, one cannot
even see the equation in a single screen any more)

numer = (
(
(
(
(
(
(
(
(
(
(
(
((AA[15] * Z + AA[14]) * Z + AA[13]) * Z
+ AA[12]
)
* Z
+ AA[11]
)
* Z
+ AA[10]
)
* Z
+ AA[9]
)
* Z
+ AA[8]
)
* Z
+ AA[7]
)
* Z
+ AA[6]
)
* Z
+ AA[5]
)
* Z
+ AA[4]
)
* Z
+ AA[3]
)
* Z
+ AA[2]
)
* Z
+ AA[1]
) * Z + AA[0]
denom = (
(
(((((ZZ + bb[14]) * ZZ + bb[12]) * ZZ + bb[10]) * ZZ + bb[8]) * ZZ + bb[6]) * ZZ
+ bb[4]
)
* ZZ
+ bb[2]
) * ZZ + bb[0]


Similarly, someone clearly thought about the formatting of:

d_r_core = 2 * amplitude * r_core ** 2 * (r_core/(r_core ** 2 + r_tide ** 2) ** (3/2) -
r_core/(r_core ** 2 + x ** 2) ** (3/2)) * \
(1./np.sqrt(r_core ** 2 + x ** 2) - 1./np.sqrt(r_core ** 2 + r_tide ** 2)) + \
2 * amplitude * r_core * (1./np.sqrt(r_core ** 2 + x ** 2) -
1./np.sqrt(r_core ** 2 + r_tide ** 2)) ** 2


Even with the back slashes, I'd take that over black's output, where
there is much less help to see what is going on (and again those lines
with just closing parentheses that standard python solves with indentation):

d_r_core = (
2
* amplitude
* r_core**2
* (
r_core / (r_core**2 + r_tide**2) ** (3 / 2)
- r_core / (r_core**2 + x**2) ** (3 / 2)
)
* (1.0 / np.sqrt(r_core**2 + x**2) - 1.0 / np.sqrt(r_core**2 + r_tide**2))
+ 2
* amplitude
* r_core
* (1.0 / np.sqrt(r_core**2 + x**2) - 1.0 / np.sqrt(r_core**2 + r_tide**2)) ** 2
)

[Thomas Robitaille]

Hi Marten,

Reading this on my mobile phone I strangely found it a lot easier to understand what was going on in both equations in the black version, in particular with so many parentheses in the first case. The indents really help see which parentheses match. It made me realize that the equation for the numerator in the first case could in fact (and should!) be written as a one or two line loop which would be a lot clearer than either current or black formatting of the current equation!

Cheers,

Tom

--
You received this message because you are subscribed to the Google Groups "astropy-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to astropy-dev...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/astropy-dev/87k0ajvfnp.fsf%40weasel.astro.utoronto.ca.

[Thomas Robitaille]

And in fact the denominator in the first case could also be written as a simple summation loop!

[Aldcroft, Tom]

On Wed, May 18, 2022 at 7:02 AM Thomas Robitaille <thomas.r...@gmail.com> wrote:

Hi Marten,

Reading this on my mobile phone I strangely found it a lot easier to understand what was going on in both equations in the black version, in particular with so many parentheses in the first case. The indents really help see which parentheses match. It made me realize that the equation for the numerator in the first case could in fact (and should!) be written as a one or two line loop which would be a lot clearer than either current or black formatting of the current equation!

Though I don't commonly code on my mobile, I had exactly the same reaction last night reading Marten's email on my mobile on both counts -- the black version was easier to read and the black reformatting gave a nice signal that the original code could be improved.

To the point of excluding blocks of hand-crafted formatting, this is *definitely* allowed and encouraged in appropriate cases. And yes, each package maintainer will want to go through all the diffs to look for cases where the reformatting is undesirable.

- Tom

 

To view this discussion on the web visit https://groups.google.com/d/msgid/astropy-dev/CAGMHX_2mCFhw-oUTWOTYw%2Bgs4VOosYPwHKAP1YH2g3Q%2BX8VKhg%40mail.gmail.com.

[Simon Conseil]

The second case could benefit from using intermediate variables both to
make the code more readable and avoid repetitive computations, as done
for other models. E.g.

r_core2 = r_core ** 2
a = r_core2 + r_tide ** 2
b = r_core2 + x ** 2
c = 1/np.sqrt(b) - 1/np.sqrt(a)
d = r_core/a ** (3/2) - r_core/b ** (3/2)

d_amplitude = r_core2 * c ** 2
d_r_core = 2 * amplitude * r_core2 * d * c + 2 * amplitude * r_core
* c ** 2
d_r_tide = (2 * amplitude * r_core2 * r_tide * c)/a ** (3/2)

(But it's probably possible to do something even better)

Simon

[William Jamieson]

Hi Martin,

I would say that the two examples you found in modeling are both difficult for me to read before and after being formatted by black. The black formatting at least made it possible for me to follow the parenthesis, which is why I decided to leave what black did in those cases alone. This however brings up a few good points:

1. One can suppress black formatting on a section of code if there is something about that section which black will break, namely wrapping the section in `# fmt: off` and `# fmt: on`. I think it is entirely reasonable to deviate from black in cases where the benefits are clear, such as visually organizing terms of an equation, lining up the entries in a "table", etc.
2. As stated and demonstrated above, applying black to a section of code which results in a truly "distasteful" formatting result is often an indication that the underlying code can/should be refactored into something better, such as creating the denominator in a loop in the first case and Simon's suggestion in the second. If black where the standard, then it would be a lot easier as a reviewer to catch, make suggestions, and correct these types of things.
3. For the general state of astropy, this also demonstrates that applying black to a section of astropy code (in this case modeling) does point out weak points in the code that we can address now in reviews of the changes made by black to the code. For example, modeling is quite large and black made a lot of changes, I welcome all those interested to review the black demonstration PR itself and suggest point out areas that black did a poor job on, so that we can either decide that the given section needs to be put back to what it was (# fmt: off/# fmt: on) or places that we can make genuine improvements to. This process can be extended to the rest of astropy if we adopt black throughout. Indeed, I posit that if we where to apply black to the whole (or parts) codebase that this will give us a chance to "re-review" much of the code and potentially improve the overall state of it.

Cheers,

William Jamieson 

[Thomas Robitaille]

Hi all,

One point that came up in the Django proposal and which I have also heard/read opinions about is whether this facilitates or hinders contributions from non-core developers. While it may seem that this requires an extra step for users making contributions, this isn't the case. At the moment, when people have made pull requests to packages I maintain (including sub-packages in astropy core) and there are PEP8 failures, I tell them they have to run:

tox -e codestyle

and then fix issues one by one (and autopep8 is not an option since it could touch other unrelated lines in the file). With auto-formatting such as (but not limited to) black, one could simply update the tox config to install and run e.g. black, which means the above command would actually do the formatting rather than just emit a whole bunch of warnings/errors to be fixed manually. We could then just tell users to commit the changes after running the above command. So this would be removing the most tedious step of style fixes.

Some people have mentioned that black is not how they personally like to code, but invoking personal coding preference is not necessarily relevant, because in a project like astropy, it is not contributor-friendly to have different styles in each sub-package, as this will confuse contributors and make it harder to know how to code.

I have to say that I was deeply against using black a few years ago when I first saw the results it produced, but I've recently started to understand that while one can often format code in a prettier or more aesthetic way by hand, the results from black often end up actually being better at conveying what is going on in the code, even if it's not as pretty - for example for function calls with many arguments one can more easily see what e.g. the fourth argument to the function is. The examples Marten sent above actually helped me understand what is going on after formatting with black, because it's really not that obvious what is going on here:

            numer = (((((((((((((((AA[15]*Z + AA[14])*Z + AA[13])*Z + AA[12])*Z + AA[11])*Z +
                               AA[10])*Z + AA[9])*Z + AA[8])*Z + AA[7])*Z + AA[6])*Z +
                          AA[5])*Z + AA[4])*Z+AA[3])*Z + AA[2])*Z + AA[1])*Z + AA[0])

whereas the black formatted code makes it clear one could simply write (untested):

    numer = AA[15]

    for idx in range(14, -1, -1):

        numer = numer * Z + AA[idx]

or could maybe even be written more simply.

While I stayed mostly neutral before in my initial message to this thread, I am changing my indicative vote to be in favor of this (with the caveat that I think we would really need as much as possible to try and minimize impact, including potentially delaying actually doing package-wide auto-formatting until 6.0 for reasons I mentioned before).

In any case, I think this is an interesting case to test out our new governance - we clearly need to now be able to make decisions without necessarily always reaching unanimity on everything!

Cheers,

Tom

--
You received this message because you are subscribed to the Google Groups "astropy-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to astropy-dev...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/astropy-dev/f2c2f8a2fc37ef0112bf391ac7d9f766dce0b0a1.camel%40sconseil.fr.

[Marten van Kerkwijk]

Thanks all for the answers. On balance, I now agree that the example I
sent is not one that ends up being a strong argument against black, and
possibly even in favour if it leads to code improvements.

But my main point really is that switching to black will actually
require work from all sub-package maintainers -- work that may well be
worth it long term, but for which time has to be found nonetheless.

Overall, count me in the neutral category (where all that's stopping me
from being actually positive is that black has all those lines with just
closing parentheses on them...).

-- Marten

[Aldcroft, Tom]

On Wed, May 18, 2022 at 9:42 AM Marten van Kerkwijk <mh...@astro.utoronto.ca> wrote:

Thanks all for the answers.  On balance, I now agree that the example I
sent is not one that ends up being a strong argument against black, and
possibly even in favour if it leads to code improvements.

But my main point really is that switching to black will actually
require work from all sub-package maintainers -- work that may well be
worth it long term, but for which time has to be found nonetheless.

Indeed, but it does not require *that* much work since the review is really a visual one. Looking at the shape of the reformatted code and detecting extreme cases of undesirable reformatting is pretty quick. Since black guarantees that the code is AST-consistent, the review does not require understanding the changes. Many of the bigger sub-packages have at least one advocate so I don't foresee review being a real issue.

 

Overall, count me in the neutral category (where all that's stopping me
from being actually positive is that black has all those lines with just
closing parentheses on them...).

Just to repeat the wise words of Tom R:  Some people have mentioned that black is not how they personally like to code, but invoking personal coding preference is not necessarily relevant, because in a project like astropy, it is not contributor-friendly to have different styles in each sub-package, as this will confuse contributors and make it harder to know how to code.

A specific example is that time, io.ascii, and table have 100 character limits, while other subpackages are 80-ish. This is basically my fault, but it is undoubtedly confusing for contributors.

- Tom

 

-- Marten

--
You received this message because you are subscribed to the Google Groups "astropy-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to astropy-dev...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/astropy-dev/y2j1qwr9cu3.fsf%40swan.astro.utoronto.ca.

[Homeier, Derek]

On 18 May 2022, at 3:42 pm, Marten van Kerkwijk <mh...@astro.utoronto.ca> wrote:

Thanks all for the answers. On balance, I now agree that the example I
sent is not one that ends up being a strong argument against black, and
possibly even in favour if it leads to code improvements.

Thanks for bringing up those examples nonetheless. I agree that the error function implementation

is not a really strong example, being essentially minimally modified from Franz Schreier’s original

single-line(!) code (which in turn, I think, was directly translated from an inline Fortran statement)

to something satisfying flake and still remaining marginally readable.

But I think it is not even very nicely readable in Fortran, and for a mobile screen rather brings up

the debate whether to change line length to 72 or 60…

Still I find it very unconvincing why something like

                                                       * Z
                                                       + AA[11]
                                                   )

should be more readable than

                                                       * Z + AA[11])

FWIW the very elegant alternative as proposed by Tom

    numer = AA[15]

    for idx in range(14, -1, -1):

        numer = numer * Z + AA[idx]

    denom = ZZ + bb[14]

    for idx in range(12, -1, -2):

        denom = denom * ZZ + bb[idx]

is in fact functionally equivalent and produces identical results (within my tests); I had specifically not

unraveled the block into an iteration since the loops at least in Fortran introduce additional overhead;

in Python that by itself seems nonmeasurable, but for very large arrays the temporary array creation does.

Incidentally I spotted an unrelated bug in applying the mask while testing this, so there will be

a PR (already one productive outcome!) where this particular example can be better discussed;

the take-away for this discussion is mainly that even suggestions for improving code coming

out of the reformatting may not be immediately applicable for numerical code like this.

Or taking up the quote from DEP-008 again:

The point of adopting an automatic code formatter isn't to have our own Django astropy-flavored code formatting style. It's about making our Python code look as much as possible like what everyone else in the Python community writes.

– is that really what we want to aspire to? Does what everyone in the numpy community, or matplotlib

developers codes, look like what everyone working on the core library, or networking, gaming or office

packages is writing, and should it better?

But my main point really is that switching to black will actually
require work from all sub-package maintainers -- work that may well be
worth it long term, but for which time has to be found nonetheless.

Overall, count me in the neutral category (where all that's stopping me
from being actually positive is that black has all those lines with just
closing parentheses on them...).

I found some other examples where black even appeared to save space (except it didn’t,

if one can count to 3), but a change like

        dg_dx_stddev = g * (-(da_dx_stddev * xdiff2 +

                              db_dx_stddev * xdiff * ydiff +

                              dc_dx_stddev * ydiff2))

–>

        dg_dx_stddev = g * (

            -(da_dx_stddev * xdiff2 + db_dx_stddev * xdiff * ydiff + dc_dx_stddev * ydiff2)

        )

definitely does not help me in understanding the structure of the code.

This just to second that the initial work is non-negligible, even if many subpackages may

be easier to deal with than `modeling`.

One more thought, when reflecting on whether I perhaps do spend significant time on

formatting decisions, my feeling is still that it is either obvious enough not to cost any

noticeable time, or it is a complex case where however time spent on the formatting

design is worth it as it should make the code clearer to follow and to modify or extend

for anyone working on it in the future.

Re. black, for me that would translate to: Is the output from black, even if not close to

the ideal, still good enough so the benefit of reducing formatting time to essentially zero

outweighs the cost of having somewhat less readable code?

I do not have any data to answer that even by my personal metrics, so waiting to hear

about some real-world experience from Django and especially scikit-learn – thanks for

trying to collect that information, Tom!

Cheers,

Derek

[Aldcroft, Tom]

It is worth emphasizing that "readability" is subjective and personal. For me, the black changes almost always *improve* readability. I suspect that with effort we might find some cognitive studies that test whether it is easier to find items in a list with one per line versus all in a comma-separated list that spans lines. My money is on one-per line, but likewise I don't have the data.

 

I do not have any data to answer that even by my personal metrics, so waiting to hear

about some real-world experience from Django and especially scikit-learn – thanks for

trying to collect that information, Tom!

I'm trying, but the email to the Django dev list has failed to get any response thus far, and my attempt to join the scikit-learn mailing list has likewise failed to this point. So... we should not count on getting useful data.

- Tom

 

Cheers,

Derek

--
You received this message because you are subscribed to the Google Groups "astropy-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to astropy-dev...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/astropy-dev/D43727CE-320A-4994-9FAA-4150A1F68E6D%40gwdg.de.

[Juan BC]

Google also uses this package https://github.com/google/yapfis less opunionated than black and has much more cobfigurations

[Moritz Guenther]

Hi,

I may have been neutral before, but this discussion (plus Derek's PR coming out of this!) have convinced me that black would beneficial for us.

Derek also seems convinced.

For Marten "all that's stopping me from being actually positive is that black has all those lines with just liberally
closing parentheses on them", so maybe he could be convinced if allowed to use "# fmt: off/on" liberally when encountering regions where that's a problem.

And I think Pey Lian never argued against black in general, only against using it on io.fits (I suggest to leave it to Simon and Pay Lian to agree on how to deal with that sub-package).

So, the only strong objections are Larry and maybe Erik.

To me that indicates that we reaching agreement without a vote. Everyone had the chance to weigh in on a public mailing list (if you are opposed to adopting black, but have not yet posted to this thread, now might be a good time!) and there is mostly agreement THAT using black is good, but the open points are implementation details like the line length and the date of adoption. Or am I missing something?

Moritz

[Brigitta Sipőcz]

Hi,

I would think that what Django and pytest do have rather little relevance, and would focus on what other math-heavy packages e.g. scipy and numpy come up with.

And as of readibilty, in my experience black-ed code loses a lot of info for nested structures when deindenting everything to the beginning of the line. I can be used to it, but the info what the extra whitespace added will be lost, yet noise is being added by spreading out stuff to soo many lines.

So, yes, autoformatting is great, but black is still not a tool I would personally use. If the project wants to be strict on formatting, there were a lot of opportunities for that in the past already when most people were against it (we could have enforced more formatting rules in CI, and with pre-commits, editors can be set up for these, too), so I feel it's just jumping from one extreme into the other gravitating towards using a tool that people only like as they don't have to come up with compromise within themselves but accepting someone else's not really ideal decisions.

Brigitta

To view this discussion on the web visit https://groups.google.com/d/msgid/astropy-dev/CAMtEP6wx-dzkp1uHsHR%2B%3Ds%2BPrbW2ndQPv84aNOtt79wrZo0GLg%40mail.gmail.com.

[Homeier, Derek]

Hi Moritz,

I may have been neutral before, but this discussion (plus Derek's PR coming out of this!) have convinced me that black would beneficial for us.

Derek also seems convinced.

for clarification, I still count myself rather as sceptically neutral. I would still agree largely to the

comments Brigitta just sent in the original thread. Functionally structured source, like having the

components of a Jacobian each on their own line as per my last example, to me still has great

value, and this is obviously something black cannot provide, probably does not even aim to do.

I’ll also confirm Tom’s point that evaluating this is highly subjective (which I tried to express as

”personal metrics”), so the final cost-benefit ratio will still be assessed differently.

As for the announced PR, I’ll have to withdraw that unfortunately (or rather not, as there was no

important case we missed in the tests after all) – I had used the original humlicek code for my tests

as a standalone function, while the version in `modeling` in contrast is using complex input and already

correctly applying the mask on that. In any case that came to light as a by-product of my benchmark

tests and not in any way from looking at the reformatted code.

For Marten "all that's stopping me from being actually positive is that black has all those lines with just liberally 
closing parentheses on them", so maybe he could be convinced if allowed to use "# fmt: off/on" liberally when encountering regions where that's a problem.

And I think Pey Lian never argued against black in general, only against using it on io.fits (I suggest to leave it to Simon and Pay Lian to agree on how to deal with that sub-package).

So, the only strong objections are Larry and maybe Erik. 

IIRC Stuart has also voiced clear opposition, plus now Brigitta.

And the discussion on code to be protected against the reformatting shows that the switch will

involve significant work, even if it only takes a quick glance for the individual statements.

To me that indicates that we reaching agreement without a vote. Everyone had the chance to weigh in on a public mailing list (if you are opposed to adopting black, but have not yet posted to this thread, now might be a good time!) and there is mostly agreement THAT using black is good, but the open points are implementation details like the line length and the date of adoption. Or am I missing something?

As noted before, the first list of pro and con voices in total already included a small fraction of the team

(which was also one point for counting them). Yes, in principle the discussion has been open long enough

for everyone with a strong opinion to weigh in, but I think this is a fundamental decision that asks for

a more formalised process, to get at least some confirmation from the other voting members that they

choose to abstain from a vote. But that indeed seems CoCo business.

Cheers,

Derek

[Aldcroft, Tom]

Here is some feedback from Guillaume Lemaître at scikit-learn. 

He notes that some new contributors don't apply black and get blocked in CI. I would argue that this is similar to our current situation with PEP8 checking, but with black the remedy is simpler: tell contributer to type `black .` and it's done. Currently they need to dig into the CI report, parse it find the errors, and fix them one-by-one by hand (and hope they got it right).

Overall he has no regrets.

- Tom

-------------------

On Wed, 18 May 2022 at 22:08, Tom Aldcroft <tald...@gmail.com> wrote:

Hi - 

The astropy core is currently considering implementing code formatting with black, much as scikit-learn did in 2020 (https://github.com/scikit-learn/scikit-learn/pull/18948). Not surprisingly there are a wide range of opinions and concerns.

My goal in writing is to request feedback from the scikit-learn community about how this transition has impacted development and community engagement in the time since the transition. 

Has there been specific feedback from contributors (both experienced and new) related to black autoformatting?

• Ease of actually running black and meeting the black-format standard for PRs. Have there been contributor problems that required assistance?

I think that this is the main drawback. One can argue that using `pre-commit` will do all the necessary changes but we still get contributors pushing without `pre-commit` being installed and stalled with an Azure CI. Since the CI could be quite overwhelming for new contributors, I could consider this part as an additional entrance barrier.

• Style considerations?

I assume that you get used to it even if anyone might get a little cranky on some styling at first. At least, it allows having any discussion regarding (some) style coding during PR reviews. I think that this is the main advantage for us.

 

• Technical problems like backports?

Not really. I think that it was quite smooth. We only get a commit in the history that is annoying while trying to blame with `git`.

 

Are there any regrets or things you would change? 

Personally not ;). We might intend to use `isort` even.

 

Do you have any advice for the astropy project?

Any feedback from your experience would be most appreciated, especially coming from a more science-oriented project like scikit-learn (as opposed to other large projects like django or pytest that have also adopted black).

Cheers,

Tom (@taldcroft on GitHub)

_______________________________________________
scikit-learn mailing list
scikit...@python.org
https://mail.python.org/mailman/listinfo/scikit-learn

[Aldcroft, Tom]

And Olivier Grisel from scikit-learn commented:

I agree with Guillaume's answers.

I think it was a net benefit, even though it might be a bit annoying
to get the tooling right for first time contributors. We can probably
improve this by making the error messages on the CI more directive on
how to fix formatting issues by given copy-pastable commands to
install and run black in your branch.

Otherwise, I really like just pressing shift-ctrl-i to fix the
formatting when editing code in VS Code.

[Moritz Guenther]

Hi,

Derek, I'm sorry I misread your position. Also, you are at least the
second person to suggest a more formal process.

I'll put this on the Coco agenda, but because 4/5 members are traveling
or in a conflicting meeting this week, the Coco might not get to it
before early June.

Personally, I see two ways to treat this in our governance model:

- We could treat this as an APE. While there is (yet) no PR to
https://github.com/astropy/astropy-APEs for black at this time, we are
discussing a pretty well defined proposal in the open as the process for
APE is intended to be (" Discussion is expected to take place using
existing mechanisms (astropy-dev, github, hangouts, etc), and eventually
a decision is made regarding whether the proposal should be accepted,
rejected, or modified."). Under these circumstances, the coordination
committee could make a decision on the de-facto APE (or someone writes a
formal APE to accept or reject) if we cannot reach a consensus or
compromise.

- We could put this to a vote to the voting members under APE0 as "Other
matters for which the Coordination Committee believes a vote is
appropriate."


However, both solutions will leave some people unsatisfied, so it would
be great if we could find some middle ground that we can all live with.
With that in mind, I went back through the thread and the notes from the
dev-telecon to distill a possible compromise:

- Black code should be autopep8 compatible, so it will pass CI. If we
agree that black formatted code is generally acceptable, then any
developer can choose to apply black FOR ANY NEW CODE if they wish.
That's close to what we do already; I'm not aware of a PR that was
turned down because someone formatted it with black.

- We already have style differences (e.g. line length) between different
sub-packages. If io.ascii is different from io.fits today anyway, maybe
io.ascii could be black formatted - it would still be different from
io.fits, just in a different way than today; maybe not ideal, but also
not much different than the current status. With that, sub-package
maintainers who like black might run their sub-packages through black at
a convenient time (when it does not mess up backports). In many
sub-packages a large fraction of PRs is done by few people, who often
are also the maintainers. If, e.g., the io.ascii maintainers agree on
black, run io.ascii through black, and use black locally for all of
their own PRs, then they get almost all the benefits of black - they
don't have to think about formatting and the formatting in the
sub-package is consistent. If there are PRs that don't follow black the
maintainers could ask if the submitter is willing to run black over it
for consistency, or add a commit just before merging themselves that
does that, or just accept that there are a few lines or functions that
are not black formatted, until running black again over the entire
module at some point in the future(e.g. for the 8.0 release).

- The last point could, but does not have to, be augmented by running
black on CI for certain sub-packages. That would ensure consistency
(which several people mentioned as a benefit), but would then require
all contributors to those packages to use black in general (with all
those points that people pointed out as possible disadvantages).

Moritz

[Jim Bosch]

On Thu, May 19, 2022 at 11:49 AM Moritz Guenther <moritz....@gmx.de> wrote:
>
> - Black code should be autopep8 compatible, so it will pass CI. If we
> agree that black formatted code is generally acceptable, then any
> developer can choose to apply black FOR ANY NEW CODE if they wish.
> That's close to what we do already; I'm not aware of a PR that was
> turned down because someone formatted it with black.
>

It may be hard to find solid data points from other projects on the
trade-offs between consistent/automatic formatting vs. barriers to
entry for contributors, but I think it's easy to find data points to
back up the statement that allowing automatic formatting in new code
blocks or files but not requiring it (across at least git-package
units) is in fact the worst of both worlds; black insists on
reformatting entire files, and hence a mixed policy adds churn to
lines otherwise unchanged by a black-formatted commit. Rubin
Observatory's use of black (required in some packages, but not others,
and well-liked everywhere it is used) vs. our use of clang-format for
C++ (permitted in all packages but required in none) is one such data
point. I've certainly heard the same from rustfmt and gofmt users.

I'm not sure Rubin's generally quite positive experience with black is
entirely relevant for Astropy's adoption, because we don't need to
prioritize lower barriers to community contributions in the same way,
and the packages where we do require black tend to be developed by
smaller teams of people with more software engineering experience than
those that do not require black. I personally find it very hard to
work in packages that _don't_ use black now, despite having once been
a deep skeptic (I shoulder much of the blame for our - in retrospect
regrettable - clang-format compromise), and I encourage anyone who
isn't a black fan to give it a try in their own projects if Astropy
does not adopt it in some form now. But I just can't speak to the
community-contribution question, and I do think that is an important
one.


Jim

[William Jamieson]

- Black code should be autopep8 compatible, so it will pass CI. If we

 

 I do not think that autopep8 is used anymore by astropy? I certainly have not used it in astropy since joining the project.

agree that black formatted code is generally acceptable, then any
developer can choose to apply black FOR ANY NEW CODE if they wish.
That's close to what we do already; I'm not aware of a PR that was
turned down because someone formatted it with black.

I think applying black to only new code is the worst possible idea, echoing Jim Bosch, black insists on formatting on a per-file basis. This means if

a contributor runs black on an existing file that they are adding new things to, then there are potentially many irrelevant changes added in addition

to those purposefully introduced. This would make reviewing PRs extremely difficult. Moreover, it can/will cause code style to diverge wildly even

within sub-packages. It is better for the whole package or at least each sub-package to apply black and then review all subsequent changes (with black

applied) to that baseline.

- We already have style differences (e.g. line length) between different
sub-packages. If io.ascii is different from io.fits today anyway, maybe
io.ascii could be black formatted - it would still be different from
io.fits, just in a different way than today; maybe not ideal, but also
not much different than the current status. With that, sub-package
maintainers who like black might run their sub-packages through black at
a convenient time (when it does not mess up backports). In many
sub-packages a large fraction of PRs is done by few people, who often
are also the maintainers. If, e.g., the io.ascii maintainers agree on
black, run io.ascii through black, and use black locally for all of
their own PRs, then they get almost all the benefits of black - they
don't have to think about formatting and the formatting in the
sub-package is consistent. If there are PRs that don't follow black the
maintainers could ask if the submitter is willing to run black over it
for consistency, or add a commit just before merging themselves that
does that, or just accept that there are a few lines or functions that
are not black formatted, until running black again over the entire
module at some point in the future(e.g. for the 8.0 release).

My PR: https://github.com/astropy/astropy/pull/13253 enables us to control application of black at the per sub-package (or even per file) level. It also

automates the process of enforcing black on those parts that want it within our existing CI; namely, the "Code style checks" job will automatically check

that all parts opted into black will have it applied. Moreover, it also provides contributors easy tooling to ensure their code is entirely compliant; namely

through the use of pre-commit (which is already used to ensure code compliance with our existing code style checks). Maybe we should add explicit

instructions to the contributor guide (aimed at new contributors) on how to use this tooling. 

I also want to point out that black can guarantee that no bugs (which is one of the concerns raised by io.fits) can be introduced (running it with the --safe option),

see

• https://black.readthedocs.io/en/stable/faq.html#is-black-safe-to-use 
• https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#ast-before-and-after-formatting

[Aldcroft, Tom]

Just one go-back on blame in GitHub, it looks like ignoring commits from a bulk change is now supported (in beta):

https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view

- Tom

[Erik Tollerud]

Hello all,

This was mentioned in another setting (dev telecon I think?), but I figured I should follow-up here so that folks are aware this didn't just fade away (fade to Black, you might say...): Tom A and I are working on an APE since it's clear this topic has a lot of meat to it, and will follow-up here when it's ready to be proposed.

---

Erik T

--
You received this message because you are subscribed to the Google Groups "astropy-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to astropy-dev...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/astropy-dev/CAMtEP6zeq_hqbgNpK2FzcVini0Tbn%3DC809toQRGBy-Pz1mN_5w%40mail.gmail.com.