-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Comments
I guess/propose to rename all |
personally, prefer 'num'. Anecdoctally, I've worked on teams that have used both and I've overheard more new folks ask about More importantly: The point that pytorch also uses For the Trainer arguments, it's a bit trickier so we might want to have some backwards compatibility like OP stated. |
Happy to standardize around Anyone want to take this as a PR? (but we need to keep backwards compatibility) |
nb --> num
and i --> idx
nb --> num
and i --> idx
@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)?
|
@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
which I think would be more natural if simplified to:
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? |
I opened a new thread to discuss the above potential variable simplifications: #584 |
I would say the the meaning is clean now but I like this proposal... =) |
i vote for making these changes now as well |
Is your feature request related to a problem? Please describe.
The code currently uses two different abbreviations for number (
nb
andnum
) and index (idx
andi
).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 thannum
, PyTorch usesnum
ubiquitously (e.g.torch.nn.RNN(num_layers=...)
), so usingnum
would probably make the API feel more familiar to users, and is the abbreviation I would suggest. I'm less sure which is better betweenidx
andi
, but it looks like there are fewer current uses ofi
, so changing those to useidx
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.
The text was updated successfully, but these errors were encountered: