Naming: open to improvements?

fastai fastai dev
1

Am I the only one who feels confused about the function/naming choices or consistency in the library?

A few examples:

Learner =>

  • opt_func / callback_fns why not opt_func / callback_funcs? or opt_fns / callback_fns?
  • Saving and loading models vs. Deploying your model: load / save vs. export / load_learner why not import ?
  • predict vs pred_batch - is abbreviation really necessary?
  • lr_find given that all(?) other methods are verbs, why lr_find and not find_lr?

The semantics of module transforms are a bit different if I understand correctly, for images is on-the-fly augmentations, but for text and tabular are really pre-processing. I found about this when I wanted to implement on-the-fly permutations to a specific column in a tabular learner and then realized the semantics of what they do are completely different than in an images.

I would be happy to give this a try and do a PR of the library with consistency changes, but given that a) naming is a religious issue for some; b) it would break compatibility completely - I wanted to know if more users feel the same or this is a non-issue… :wink:

6 Likes
2

My view is that changing names to make them internally consistent is a good idea. I’d be most happy to look at a PR - although it would need to include a PR for the course-v3 notebooks and docs too, so it’s quite a big of work!

1 Like
3

@antorsae maybe try automating in one script as much as possible

  1. pull down from git: https://github.com/fastai/fastai/blob/master/tools/fastai-make-pr-branch-py
  2. list / replace names with a script and make a report of changes into a file
  3. run all tests pytest
  4. test notebooks (think thats possible?): https://github.com/zonca/pytest-ipynb
  5. manual code (functionality?) check, based on report
  6. push

its gonna be fun though in any case, and ongoing PRs need to wait a moment. So maybe one PR per name change?

But if you wanna change probably rather sooner than later (before code in production instances e.g. )

3 Likes
4

Not only possible, but already set up and working :slight_smile: This is the script to run:

1 Like
5

Could you consider a fairly substantial change to something like this which will break a lot of scripts people have written as a 1.1 release rather than just pushing it out?

I know it would break a lot of code I’ve written.