Coding Style for v1


(Russell R Cohen) #1

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 Howard) #2

Covered in:


(Russell R Cohen) #3

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.


(Michael Skinner) #4

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 Howard) #5

Indeed you have, thank you. :slight_smile:

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.