Coding Style for v1

[rcoh (Russell R Cohen)]

I’m sure this is been hashed out many times – the creation of fastai v1 seems as good a time as any to give it another go. I’m excited to see that fastai_v1 is targetting 5 letter acronyms instead of 3 letter ones, definitely an improvement.

The coding style used in fastai of squeezing as much code as possible on a line is an interesting one:

Code like that would be pretty much unheard of in a professional software engineering setting – Without passing judgement on whether code like:

    if not isinstance(n_epochs, Iterable): n_epochs = [n_epochs]
    if not isinstance(data, Iterable): data = [data]
    if len(data) == 1: data = data * len(n_epochs)
    for cb in callbacks: cb.on_phase_begin()
    model_stepper = stepper(model, opt.opt if h

is more or less readable than:

    if not isinstance(n_epochs, Iterable): 
        n_epochs = [n_epochs]
    if not isinstance(data, Iterable):
        data = [data]
    if len(data) == 1:
        data = data * len(n_epochs)
    for cb in callbacks:
        cb.on_phase_begin()

I don’t think many would disagree that to most people coming to fastai from a software engineering background, the second is much more readable, just because thats how code like that is typically fomatted.

By using autoformatters which turn code like:

label2idx = {v:k for k,v in enumerate(all_labels)}

into

label2idx = {v: k for k, v in enumerate(all_labels)}

It’s just easier for people you are used to reading autoformatted code to parse. The whitespace is a visual signal for what’s going on and it’s critical for understanding code at a glance.

It comes down to who the target audience of fastai is. If it’s professional software engineers getting their toes wet in data science, then I think it makes a lot of more sense to make fastai look like the style of Python engineers are used to reading.

If the goal is to help grad students and data scientists get into deep learning, than perhaps the current style is fine.

I would propose the following:

1. Use an autoformatter on fastai_v1 from the beginning.
2. Push the variable names a little further towards being descriptive, especially in places where there is no context.

Take this code:

class RandomLighting(Transform):
    def __init__(self, b, c, tfm_y=TfmType.NO):
        super().__init__(tfm_y)
        self.b,self.c = b,c

I have a hard time believing that it would be harder to read and understand as balance and contrast instead of b and c.

[jeremy (Jeremy Howard)]

Covered in:

About the fastai dev category

We’re doing a rewrite of the fastai library, with the following goals: Support consistent API for classification, regression, localization, and generation, across all of: vision, NLP, tabular data, time series, and collaborative filtering Clear and complete documentation for both new and experienced users Well tested (both unit and integration tests) Better structured code Notebooks showing how and why the library is built as it is. The repo is here: If you’re interested in helping, feel …

[rcoh (Russell R Cohen)]

That’s fair and I’m embarrassed to admit that I missed the bottom of that post – I wouldn’t have sent this if I’d read it. Looking forward to fast_ai v1. Once you’re ready for more contributors I’d be happy to lend many years of Python OO experience and a decent working knowledge of fast.ai internals.

[mcskinner (Michael Skinner)]

Now that I’ve started out by solving some problems with my contributions…

I am like @rcoh, in that standard formatting makes it a lot easier for me to read code at a glance. I’ve been programming for 25 years across at least a dozen languages, and my current day job has my doing a lot of code reviews. I read a few hundred lines of code a day, in 3 core languages, and conventions help a lot.

Then again one of my most frequent asks is for people to make better use of vertical whitespace, so I side with the fastai conventions in some respects. There also isn’t quite so much fastai code to read. I would say I care more about importing style, since I’ve been burnt by the import * from some_module convention (naming collisions between old and new NLP files).

[jeremy (Jeremy Howard)]

Now that I’ve started out by solving some problems with my contributions

Indeed you have, thank you.

I would say I care more about importing style, since I’ve been burnt by the import * from some_module convention (naming collisions between old and new NLP files).

You may want to check the previous discussions first, since I think your issue is already dealt with there.

Recommendation: replace import * with 'import as

I would like to suggest to discourage the usage of import *. Instead use import with aliases. Eg it is very common for us to write: import numpy as np. I suggest similar style should be encouraged everywhere instead of import *. It provides better readability of the code, avoids namespace pollution and naming conflicts etc.

[stas (Stas Bekman)]

Observing our PR review notes (mine and others), In an effort to be less of a hypocrite when asking PR submitters to adhere to our code style, and so that our code can act as monkey-see-monkey-do style guide, we need to sort out some inconsistencies:

Currently inconsistent style:

1. we use half the time each of:

   if foo==bar:
   if foo == bar:
   

   it should be the second way

2. we use:

   foo=bar
   foo = bar
   

   it should be the second way,

   unless it’s a func argument, then it’s

   func(foo=True, bar=False)
   

3. we use:

   func(foo,bar, tar)
   func(foo, bar, tar)
   

   it should be the second way

4. since we seem to need to stick with: func(foo, bar, tar), there is a conflict with these 2 styles:

   foo,bar,tar = a,b,c
   foo,bar,tar = func(a,b,c)
   

   since it should be:

   foo,bar,tar = func(a, b, c)
   

   but then this looks again inconsistent:

   foo,bar,tar = a,b,c
   foo,bar,tar = func(a, b, c)
   for i,x in zip(foo, bar)
   

5. the style says:

   f"this {foo} bar"
   

   but we still have:

   "this "+ foo + " bar"
   

I just applied these changes to: https://github.com/fastai/fastai/commit/6c71bcafeda64b15b9961c3fbfbcd34af82c16fa

the only thing I don’t know how to resolve is the conflict between 2 styles in item 4 above.

update: I run these by Jeremy and he clarified that all these are good and there is no strict requirement other than what’s in the style guide: https://docs.fast.ai/dev/style.html, which doesn’t include any of the items I listed in this post.

So consider the above Stas’ style guide and this is how I write my code, but you can do it in any other way while adhering to the style guide (link above).

and I sorted out case 4 for my own preferences - func args are always space separated and the only time assignment has no space is when groups are assigned (not results of func) and in loops:

foo, bar = func(a, b, c)
foo,bar = a,b
foo,bar = funca(),funcb() # don't do it
foo,bar = funca(a, b, c),func(1, 2, 3) # neither this
for i,x in zip(foo, bar)

no more conflict there.

[kechan (kelvin chan)]

Thanks for highlighting coding style. I think It may be great to highlight common mistake vs. right way by a red vs. green checkmark in a doc…