-
Notifications
You must be signed in to change notification settings - Fork 92
Update MultiSolverInterface #520
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
Conversation
Code Coverage SummaryResults for commit: bfa7ebe Minimum allowed coverage is ♻️ This comment has been updated with latest results |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @dario-coscia for this PR.
I agree on the proposed changes, but I do not like how you make the user aware of manual optimization. In my opinion, introducing a bug on purpose does not look like the cleanest way to achieve this.
What about a note in the documentation?
I propose something like this: """
Docstring...
.. note::
When creating a subclass of :class:`MultiSolverInterface`, ensure that `automatic_optimization`
is set to `False`, and manually handle backpropagation, as well as optimizer and scheduler steps.
"""What do you think, @ndem0? |
By default, lightning sets that variable equal to true, which I agree with since we want automatic optimization by default. I can remove it but in the future, if PyTorch lightning decides to change it, it will cause unexpected behaviour in the Multisolver |
I see your point! What about setting it directly to |
The problem of setting it to |
I understand. However, I am positive a note in the documentation should suffice. I suggest to wait for other opinions, so as to find the best option. Meanwhile, thank you for the clarification! |
|
@ndem0 We have a discussion going with @GiovanniCanali (see above) on this line here: Line 505 in 2841ab5
What do you think it might be the best option? Thank you! |
f8d0d52 to
a92277e
Compare
@GiovanniCanali @FilippoOlivo I talked quickly with @ndem0 on the issue to get feedback, but I had trouble implementing a runtime warning. Therefore, I set |
GiovanniCanali
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks fine for me!
I am ok with this solution! |
|
@GiovanniCanali if you are ok with it please approve the PR for merging |
Fixes #519