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

Simplify variables: step_idx -> step, epoch_idx -> epoch, max_num_epochs -> max_epochs, min_num_epochs -> min_epochs #584

Closed
elliotwaite opened this issue Dec 4, 2019 · 8 comments · Fixed by #589
Labels
feature Is an improvement or enhancement help wanted Open to be worked on

Comments

@elliotwaite
Copy link
Contributor

elliotwaite commented Dec 4, 2019

Is your feature request related to a problem? Please describe.
This is a feature request to simplify these variables:
step_idx, epoch_idx, max_num_epochs, and min_num_epochs

To:
step, epoch, max_epochs, and min_epochs

These variables were all recently updated by change #567, so I thought if these variables were going to simplified, now would be a good time to do so.

To me, using batch_idx makes sense since it's the index value for the current batch in a sequence of batches, but since step and epoch aren't index values for a sequence, but rather counters, using the simplified versions of these without the _idx suffix makes more sense to me.

Using the simplified max_epochs and min_epochs is more just based on what seems more natural to me, so I'd be interested to hear if others also agreed that these would be good changes to make.

@elliotwaite elliotwaite added feature Is an improvement or enhancement help wanted Open to be worked on labels Dec 4, 2019
@elliotwaite elliotwaite changed the title Simplify Variables (step_idx -> step, epoch_idx -> epoch, max_num_epochs -> max_epochs, min_num_epochs -> min_epochs) Simplify variables: step_idx -> step, epoch_idx -> epoch, max_num_epochs -> max_epochs, min_num_epochs -> min_epochs Dec 4, 2019
@Borda
Copy link
Member

Borda commented Dec 4, 2019

@williamFalcon the changes are here now...

@williamFalcon
Copy link
Contributor

max and min epochs?

@Borda
Copy link
Member

Borda commented Dec 4, 2019

I would consider even unify min and max in single variable range

@elliotwaite
Copy link
Contributor Author

@Borda Can you expand on this idea. Would it be a tuple? Would the variable be named epochs_range? What would that look like when passed in as a command-line argument?

@Borda
Copy link
Member

Borda commented Dec 5, 2019

Yeah, tuple with name epoch_interval and via command line you would pass two numbers which is trivial =)

@elliotwaite
Copy link
Contributor Author

Got it. Personally, if I saw epoch_interval without explanation, it would not be very clear to me what it meant. I might wonder if it was for some kind of interval happening within each epoch, or if it was the counter for which epoch interval we were currently in. epochs_range is similarly less clear to me than max_epochs and min_epochs.

Also, I would imagine for a majority of use cases, users will only want to specify a maximum number of epochs, and that also specifying a minimum will be less common, which makes me lean towards keeping them as separate variables to allow users to easily omit specifying the minimum.

Having them tied together in a tuple does make sense in that they are tightly related, but in my experience, it seems more common practice to just have two separate int variables with the same name except for the min and max part (but common practice is not necessarily better practice).

@williamFalcon
Copy link
Contributor

agreed. let’s keep it min max as the range thing is confusing

@Borda
Copy link
Member

Borda commented Dec 5, 2019

@elliotwaite good, are going to realize it is a PR?

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.

3 participants