Update callbacks and tests #482
Conversation
| """PINA Callbacks Implementations""" | ||
|
|
||
| # To remove once Callbacks are implemented again. | ||
| # pylint: disable=W0611 |
There was a problem hiding this comment.
I would remove this for now. Maybe, we will restore them in the future but for now I would keep all the warnings
pina/callback/optimizer_callback.py
Outdated
| self._new_optimizers = new_optimizers | ||
| self._epoch_switch = epoch_switch | ||
|
|
||
| # pylint: disable=W0212 |
There was a problem hiding this comment.
Also here! I suggest to remove this
| for idx, optim in enumerate(self._new_optimizers): | ||
| optim.hook(trainer.solver.models[idx].parameters()) | ||
| optims.append(optim.instance) | ||
| optim.hook(trainer.solver._pina_models[idx].parameters()) |
There was a problem hiding this comment.
We are accessing a private member of the solver class. Isn't a cleaner way to do the same thing?
There was a problem hiding this comment.
Nope, this is the only way to work for all solvers.
GiovanniCanali
left a comment
GiovanniCanali
left a comment
There was a problem hiding this comment.
Thank you, @dario-coscia, for fixing the callbacks. Code looks good to me!
While I believe the docstrings will be addresses in the corresponding PR, I am leaving some comments to point out some minor inconsistencies.
I am also committing some small rephrasing modifications and typos corrections.
pina/callback/processing_callback.py
Outdated
| """ | ||
| Lightning Callback for Metric Tracking. | ||
| Tracks specific metrics during the training process. |
There was a problem hiding this comment.
I suggest to move the description of callbacks to the class docstring, and fill the docstring of __init__ methods with brief sentences, such as "Initialization" for instance. What do you think?
pina/callback/processing_callback.py
Outdated
| :param metrics_to_track: List of metrics to track. Defaults to train/val loss. | ||
| :param metrics_to_track: List of metrics to track. | ||
| Defaults to train loss. | ||
| :type metrics_to_track: list, optional |
There was a problem hiding this comment.
In some cases, the type list is used generically, while in others, the specific types of instances within the list are explicitly defined. What convention should we follow?
There was a problem hiding this comment.
Thank! You are right it should be list of str
|
Can we merge? |
|
Ok for me! |
dario-coscia
force-pushed
the
0.2
branch
2 times, most recently
from
1f54775 to
795e4a4
Compare
|
Yes! |
|
Ready to merge! |
FilippoOlivo
force-pushed
the
0.2
branch
3 times, most recently
from
7eec394 to
795e4a4
Compare
--------- Co-authored-by: giovanni <[email protected]>
--------- Co-authored-by: giovanni <[email protected]>
--------- Co-authored-by: giovanni <[email protected]>
--------- Co-authored-by: giovanni <[email protected]>
--------- Co-authored-by: giovanni <[email protected]>
--------- Co-authored-by: giovanni <[email protected]>
--------- Co-authored-by: giovanni <[email protected]>
In this PR the PINA callbacks and related tests are updated.
The Refinment callback is deprecated, see issue #481