- Notifications
You must be signed in to change notification settings - Fork 3.6k
Description
Is your feature request related to a problem? Please describe.
The code currently uses two different abbreviations for number (nb and num) and index (idx and i).
An example of nb:
def training_step(self, batch, batch_nb):
https://williamfalcon.github.io/pytorch-lightning/LightningModule/RequiredTrainerInterface/#training_step
An example of num:
def log_metrics(self, metrics, step_num):
https://williamfalcon.github.io/pytorch-lightning/Trainer/Logging/#custom-logger
An example of idx:
def training_step(self, batch, batch_nb, optimizer_idx):
https://williamfalcon.github.io/pytorch-lightning/LightningModule/RequiredTrainerInterface/#training_step
An example of i:
def optimizer_step(self, current_epoch, batch_nb, optimizer, optimizer_i, second_order_closure=None):
https://williamfalcon.github.io/pytorch-lightning/Trainer/hooks/#optimizer_step
Describe the solution you'd like
I think switching to using consistent abbreviations for both number and index would improve the design quality of the API.
Although nb is currently used more than num, PyTorch uses num ubiquitously (e.g. torch.nn.RNN(num_layers=...)), so using num would probably make the API feel more familiar to users, and is the abbreviation I would suggest. I'm less sure which is better between idx and i, but it looks like there are fewer current uses of i, so changing those to use idx would probably be easier.
I know one of Lightning's design principles is a backward-compatible API, but there are some options for updating argument names with deprecation warnings that could be considered:
https://stackoverflow.com/questions/49802412/how-to-implement-deprecation-in-python-with-argument-alias
Also if this change is ever going to be made, the sooner probably the better, while Lightning is still in its relatively early stages of development.
Describe alternatives you've considered
Alternatively, if this would be too much of a breaking change, perhaps this change could be delayed until when a future breaking change is released.
I'm interested to hear others' opinions on this.