docs: drop redundant defaults #16097
Conversation
Borda
requested review from
awaelchli,
carmocca,
justusschock,
tchaton and
williamFalcon
as code owners
⚡ Required checks status: All passing 🟢Groups summary🟢 pytorch_lightning: Tests workflowThese checks are required after the changes to 🟢 pytorch_lightning: Azure GPU
These checks are required after the changes to 🟢 pytorch_lightning: Azure HPU
These checks are required after the changes to 🟢 pytorch_lightning: Azure IPU
These checks are required after the changes to 🟢 pytorch_lightning: Docs
These checks are required after the changes to 🟢 mypy
These checks are required after the changes to 🟢 installThese checks are required after the changes to Thank you for your contribution! 💜
|
| trying to optimize initial learning for faster convergence. trainer.tune() method will | ||
| set the suggested learning rate in self.lr or self.learning_rate in the LightningModule. | ||
| To use a different key set a string instead of True with the key name. | ||
| Default: ``False``. |
There was a problem hiding this comment.
Some of these were added by contributors: #11614
As a general guideline I agree, but it also doesn't hurt having them in something like the core Trainer API (that doesn't really change their defaults that often).
I propose to keep these removals to a minimum, and to keep them in the Trainer.
There was a problem hiding this comment.
I see, not sure why we moved some back...
In my point of view, I am very fine to keep the default when there is some additional explanation or referring to the class type or just in text after a value set that the one is default... but having pure sentence "Default: None." I think is useless...
There was a problem hiding this comment.
I suggest keeping the "default:" message for the arguments that are Optional in the code only so that we can know whether the user manually passed an argument or not to then set the real default. All others could be removed
There was a problem hiding this comment.
arguments that are Optional in the code only so that we can know whether the user manually passed an argument
not sure if I understand what you mean here 😕
There was a problem hiding this comment.
We often use the pattern
x: Optional[int]
if x is None:
# the user didn't set it
self.x = 10
else:
self.x = xinstead of
x: int = 10
self.x = xSo for those, we want to still mention Default: 10 in the docstring because saying Default: None is not informative unless it also explains that None gets converted into 10
There was a problem hiding this comment.
yes agree, but so far it was not such and we just said default None...
There was a problem hiding this comment.
I'm ok with adjusting the other less important files. But my opinion remains unchanged regarding trainer.py, I'd like that it stays and that we don't go back and forth #11614
There was a problem hiding this comment.
But @Borda how about we merge the PR but remove the changes from Trainer. That's what I'm trying to say.
4 participants
What does this PR do?
these defaults are already shared as the footprint of function/method so having then in docs is duplicate and also not often updated
Before submitting
PR review
Anyone in the community is welcome to review the PR.
Before you start reviewing, make sure you have read the review guidelines. In short, see the following bullet-list:
Did you have fun?
Make sure you had fun coding 🙃
cc @Borda