-
Notifications
You must be signed in to change notification settings - Fork 91
Switch scheduler callback #735
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
ae7f36c to
41e6e51
Compare
41e6e51 to
3981c13
Compare
|
|
||
| Processing callback <callback/processing_callback.rst> | ||
| Optimizer callback <callback/optimizer_callback.rst> | ||
| Switch Scheduler <callback/switch_scheduler.rst> |
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.
I would keep the Scheduler callback, so that in the future, we can add other callbacks there
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.
This differs from #445, where we chose to name each file after the class it contains.
That said, your observation raises a valid point. My proposal is the following: within the callback directory, we create a dedicated scheduler subdirectory that, for now, will include only switch_scheduler.py, while leaving room for future additions. This prevents the file from becoming overly long as more scheduler-related callbacks are introduced.
The same structure would apply to optimizer_callback and processing_callback. In particular, the layout I have in mind is the following:
-- callback
-------- scheduler_callback
--------------- switch_scheduler.py
-------- optimizer_callback
--------------- switch_optimizer.py
--------- refinement
--------------- refinement_interface.py
--------------- r3_refinement.py
-------- processing_callback
--------------- metric_tracker.py
--------------- pina_progress_bar.py
--------------- normalizer_data_callback.py
Happy to hear your thoughts on this, @dario-coscia @FilippoOlivo @ndem0.
All these changes would be implemented in a dedicated PR.
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.
-- callback
-------- scheduler
--------------- switch_scheduler.py
-------- optimizer
--------------- switch_optimizer.py
--------- refinement
--------------- refinement_interface.py
--------------- r3_refinement.py
-------- processing
--------------- metric_tracker.py
--------------- pina_progress_bar.py
--------------- normalizer_data_callback.py
I prefer something like this (just remove callback on the subfolders)
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.
Very good! Maybe I would fuse scheduler and optimizer in optim directory (this is similar to from pina.optim import ...)
Description
This PR fixes #734
Checklist