Deprecate LossRecorder, create LossOutput dataclass alternative
#1749
Conversation
This reverts commit 5905efb.
Codecov ReportBase: 90.86% // Head: 90.85% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1749 +/- ##
==========================================
- Coverage 90.86% 90.85% -0.01%
==========================================
Files 118 118
Lines 9969 10015 +46
==========================================
+ Hits 9058 9099 +41
- Misses 911 916 +5
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
scvi/train/_trainingplans.py
Outdated
| @@ -281,7 +284,7 @@ def forward(self, *args, **kwargs): | |||
| @torch.inference_mode() | |||
| def compute_and_log_metrics( | |||
| self, | |||
| loss_recorder: LossRecorder, | |||
| loss_recorder: Union[LossRecorder, LossOutput], | |||
| metrics: MetricCollection, | |||
There was a problem hiding this comment.
ordereddict now? to ensure the next(iter stuff works
There was a problem hiding this comment.
this is still the loss output
There was a problem hiding this comment.
commenting on the metrics not the loss recorder
scvi/train/_trainingplans.py
Outdated
| # Use the torchmetric object for the ELBO | ||
| # We only need to update the ELBO metric | ||
| # As it's defined as a sum of the other metrics | ||
| next(iter(metrics.values()))[0].update( |
There was a problem hiding this comment.
why can't you key into elbo here?
There was a problem hiding this comment.
ah yeah I didn't see we have access to the mode here
| reconstruction_loss: Optional[LossRecord] = None | ||
| kl_local: Optional[LossRecord] = None | ||
| kl_global: Optional[LossRecord] = None | ||
| extra_metrics: Optional[Dict[str, Tensor]] = field(default_factory=dict) |
There was a problem hiding this comment.
| extra_metrics: Optional[Dict[str, Tensor]] = field(default_factory=dict) | |
| extra_metrics: Dict[str, Tensor] = field(default_factory=dict) |
There was a problem hiding this comment.
I would say this is optional because you don't need to specifiy it
Dataclasses allow the same workflow to be applied to jax-based models.
Two changes are important:
LossRecorderis deprecated, to be removed in v0.20.0JaxTrainingPlangets proper logging using the same paradigmTODO:
LossOutputSome code was added to loss recorder to make it backwards compatible, but it will be removed anyway