Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make all abbreviations: (nb --> num) and (i --> idx) #534

Closed
elliotwaite opened this issue Nov 21, 2019 · 8 comments · Fixed by #567
Closed

Make all abbreviations: (nb --> num) and (i --> idx) #534

elliotwaite opened this issue Nov 21, 2019 · 8 comments · Fixed by #567
Labels
feature Is an improvement or enhancement help wanted Open to be worked on

Comments

@elliotwaite
Copy link
Contributor

elliotwaite commented Nov 21, 2019

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.

@elliotwaite elliotwaite added feature Is an improvement or enhancement help wanted Open to be worked on labels Nov 21, 2019
@Borda
Copy link
Member

Borda commented Nov 21, 2019

I guess/propose to rename all ..._i to ..._idx and also it makes sense to unify the nb and num
@williamFalcon @Ir1d @jeffling do you have a preference about nb/mun e.g. sklearn is mostly using num but I personally prefer nb

@elliotwaite elliotwaite changed the title Consistent abbreviation for number (nb vs num) and index (idx vs i) Consistent abbreviations for number (nb vs num) and index (idx vs i) Nov 21, 2019
@jeffling
Copy link
Contributor

jeffling commented Nov 22, 2019

personally, prefer 'num'. Anecdoctally, I've worked on teams that have used both and I've overheard more new folks ask about nb over num :)

More importantly: The point that pytorch also uses num is very convincing, seeing as Lightning should feel as familiar as possible to any researcher coming into the codebase. I think that hits the design principles.

For the Trainer arguments, it's a bit trickier so we might want to have some backwards compatibility like OP stated.

@williamFalcon
Copy link
Contributor

Happy to standardize around num if pytorch uses it.
All the i should become idx.

Anyone want to take this as a PR? (but we need to keep backwards compatibility)

@williamFalcon williamFalcon changed the title Consistent abbreviations for number (nb vs num) and index (idx vs i) Make all abbreviations: nb --> num and i --> idx Nov 25, 2019
@williamFalcon williamFalcon changed the title Make all abbreviations: nb --> num and i --> idx Make all abbreviations: (nb --> num) and (i --> idx) Nov 25, 2019
@Borda
Copy link
Member

Borda commented Nov 25, 2019

@williamFalcon do you want to hold back compatibly for how long? for several next releases and then clean the API or forever (this will make the API quite messy in the long term)?
I would make it like

warnings.warn("`root_module` module has been renamed to `lightning` since v0.5.3"
              " and will be removed in v0.8.0", DeprecationWarning)

@elliotwaite
Copy link
Contributor Author

@williamFalcon @Borda Sorry for the delay in reply, I was subscribed to too many GitHub issues. Those changes look good to me! I also like the switch from batch_num to batch_idx. The only variables that now stand out to me when looking over the PR are:

step_idx, epoch_idx, max_num_epochs, andmin_num_epochs

which I think would be more natural if simplified to:

step, epoch, max_epochs, and min_epochs

This, however, is a separate issue than the choice of abbreviation, so perhaps this should be discussed in a separate thread if it's going to be discussed at length. But since those variables are being updated, perhaps addressing these at the same time would be a good idea if any of them are going to be changed. Any thoughts on these potential simplifications?

@elliotwaite
Copy link
Contributor Author

I opened a new thread to discuss the above potential variable simplifications: #584

@Borda
Copy link
Member

Borda commented Dec 4, 2019

I would say the the meaning is clean now but I like this proposal... =)

@williamFalcon
Copy link
Contributor

i vote for making these changes now as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Is an improvement or enhancement help wanted Open to be worked on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants