Logging additional metrics #448
Conversation
Coverage reportNote Coverage evolution disabled because this PR targets a different branch The coverage rate is
Diff Coverage details (click to unfold)src/renate/updaters/model_updater.py
src/renate/utils/misc.py
|
| self._log(trainer=trainer, training=pl_module.training) | ||
| self._log(trainer=trainer, pl_module=pl_module) | ||
|
|
||
| def on_train_start(self, trainer: Trainer, pl_module: LightningModule) -> None: |
There was a problem hiding this comment.
is this called also for sanity_checking?
There was a problem hiding this comment.
It runs on on_validation_start, hence the special handling checking for None in the Additional metrics class.
src/renate/utils/misc.py
Outdated
|
|
||
| def __call__(self, model: torch.nn.Module) -> Dict[str, Union[float, int]]: | ||
| if self._training_times: | ||
| epoch_training_time = self._training_times[-1] - self._training_times[0] |
There was a problem hiding this comment.
why is this the epoch_training_time? isn't this total training time?
src/renate/utils/misc.py
Outdated
| def on_train_start(self) -> None: | ||
| self._training_times.append(time.time()) | ||
|
|
||
| def on_train_epoch_end(self) -> None: | ||
| self._training_times.append(time.time()) |
There was a problem hiding this comment.
instead of using a list, maybe simply save start and end time?
There was a problem hiding this comment.
I just wanted to keep the flexibility of retrieving all times, though the current interface doesn't
necessarily allow for it.
There was a problem hiding this comment.
unless we use it or plan to use it soon, we shouldn't sacrifice simplicity for flexibility. unless you strongly disagree, let's keep this simple.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.