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

Rename LighterWriter's arguments #47

Closed
ibro45 opened this issue Mar 8, 2023 · 1 comment
Closed

Rename LighterWriter's arguments #47

ibro45 opened this issue Mar 8, 2023 · 1 comment
Labels
enhancement New feature or request

Comments

@ibro45
Copy link
Collaborator

ibro45 commented Mar 8, 2023

🚀 Feature Request

write_dir -> save_dir, write_interval -> interval, write_format -> ?. Would be good not to override the built-in format. Yes, we are overriding the built-in input throughout the code, but the community's used to it because PyTorch does that. Furthermore, almost no one ever uses input(), whereas string formatting with format() isn't uncommon.

🔈 Motivation

LighterWriter's name says it's a writer, no need to repeat that in the arguments.

🛰 Alternatives

Override the built-in format, or come up with a synonym for format.

@ibro45 ibro45 added the enhancement New feature or request label Mar 8, 2023
@surajpaib
Copy link
Collaborator

surajpaib commented Mar 9, 2023

I don't necessarily see the issue with using format. The format used for strings is a method for the string class and should be available irrespective of us using the name in the class. We will never actually be overriding it.

Given the context, I also think it isn't confusing to use it as it's obvious we are not referring to the method format of string

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants