8 ms·
I really like the fastai abstractions and their attention to detail. Also their callbacks are amazing. I always look at their implementations for inspiration.
by ya3r 7y ago
I really like the fastai abstractions and their attention to detail. Also their callbacks are amazing. I always look at their implementations for inspiration.
But my main issue with it is the code formatting (https://docs.fast.ai/dev/style.html#layout https://docs.fast.ai/dev/style.html#layout).
Maybe I am too used to PEP-8 and black (https://github.com/psf/black https://github.com/psf/black) formatted code, but honestly I cannot stand the code format.
- dr_zoidberg 7y agoUfff that's rough to read. Totally. The only one I agree with is the one that says: Aim to align statement parts that are conceptually similar. It allows the reader to quickly see how they’re different. E.g. in this code it’s immediately clear that the two parts call the same code with different parameter orders. That'd turn something like this: class OneClass: def __init__(self, a, b1, b2, c_long): self.a = a self.b1 = b1 self.b2 = b2 self.c_long = c_long Into this: class OneClass: def __init__(self, a, b1, b2, c_long): self.a = a self.b1 = b1 self.b2 = b2 self.c_long = c_long (maybe not the greatest example, there are places where this helps much more) The others all are un-pythonic and make the code more unreadable.
- claytonjy 7y agoI've long wished black would do this!
- dr_zoidberg 7y agoWhile I appreciate what black can do (no more discussions about code style!) I am lucky enough that I manage a small team (2-5 programmers) that understand and follow the style convention we set.
- jph00 7y agoIt's certainly unpythonic - as the link explains, it's based on research that goes back many more decades than Python has existed, and that PEP 8 entirely ignored. But it only makes the code unreadable if you don't make a tiny effort to adjust. If you do make the effort, there's some great payoff, like this code: try: self._split(b); self('begin_batch') self.pred = self.model(*self.xb); self('after_pred') if len(self.yb) == 0: return self.loss = self.loss_func(self.pred, *self.yb); self('after_loss') if not self.training: return self.loss.backward(); self('after_backward') self.opt.step(); self('after_step') self.opt.zero_grad() except CancelBatchException: self('after_cancel_batch') finally: self('after_batch') That's the inner part of the training loop. You can see at a glance: what steps are in the loop; what callbacks are in the loop, in what order; what step corresponds to each callback. And you can see the whole training loop at once, which is great for getting a clear picture of what's going on.
- dr_zoidberg 7y agoBy looking at that code without knowing much of the context in which it works, I assume self('begin_batch') and the like are "signals" that set some state or are used for logging. That behaviour could be achieved using other mechanisms (perhaps some metaprogramming magic or an observer pattern). And while I can appreciate it can be quick to see where the signals are sent, the use of ; and having two things in a line still aren't convincing me. Even more, if I were to run the line_profiler here, I know it'd report weird numbers precisely for having more than one thing per line. The other thing that I dislike is opening blocks and closing them in the same line. It may be force of habit for me, but that screams unreadability at my face. Rounding up, all I see is behaviour that can be achieved through other mechanisms, and dev/tools unfriendliness. And notice I'm not sayin anything about PEP-8, because: a) There are parts of it with which I don't agree either. b) Many people use PEP-8 as a sort of "silver bullet" and argument-ending-remark. That's not what it should be, it should be a _guide_ to be used when it helps, and ignored sparingly (with reason and consideration of _why_ you decide to ignore it, in the sake of readability).
- jph00 7y agoEven the creator of Python says "Black is good for large projects, but I find it uglifies all code compared to manually formatting..."