Conversation
janfb
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1861 +/- ##
==========================================
+ Coverage 87.83% 87.86% +0.02%
==========================================
Files 140 140
Lines 12958 12989 +31
==========================================
+ Hits 11382 11413 +31
Misses 1576 1576
Flags with carried forward coverage won't be shown. Click here to find out more.
|
dgedon
left a comment
dgedon
left a comment
There was a problem hiding this comment.
The input argument structure with overlay is not yet clean or needs some clarification at least. Also, is there supposed to be a how-to-guide attached to it?
| plot_kwargs: Optional[Dict[str, Any]] = None, | ||
| verbose: bool = True, | ||
| tensorboard_scalar_limit: int = 10_000, | ||
| inference: Union[NeuralInference, Path, None] = None, |
There was a problem hiding this comment.
Potentially add a comment about deprecation of this variable in the future
There was a problem hiding this comment.
it's marked as deprecated in the docstring below.
| for name, vals in (("colors", colors), ("labels", labels), ("ylabel", ylabel)): | ||
| if vals is not None and len(vals) != len(tags): | ||
| raise ValueError( | ||
| f"`{name}` must have the same length as `tags` " | ||
| f"(got {len(vals)} vs {len(tags)})." | ||
| ) |
There was a problem hiding this comment.
Potentially first collect which vals are of the wrong length and then give a joint error about all mis-lengthed vals at once.
There was a problem hiding this comment.
Isn't there the same condition on axes if overlay=False?
There was a problem hiding this comment.
on collecting vals lenghts first, I think it's fine to just do it on each iteration in the loop, it's only three times.
The other point is fixed with the recent commit.
| overlay: if True, plots all ``tags`` on a single axes (useful for | ||
| comparing training vs validation loss). Otherwise one subplot per tag. |
There was a problem hiding this comment.
What happens if overlay=True and we have multiple axes as arguments? It seems that it ignores multple axes then from the code. Should be specified
There was a problem hiding this comment.
good catch, there are multiple combinations of overlay and passing axes that will fail here. I will push a fix.
| plot_kwargs: Optional[Dict[str, Any]] = None, | ||
| verbose: bool = True, | ||
| tensorboard_scalar_limit: int = 10_000, | ||
| inference: Union[NeuralInference, Path, None] = None, |
There was a problem hiding this comment.
it's marked as deprecated in the docstring below.
| overlay: if True, plots all ``tags`` on a single axes (useful for | ||
| comparing training vs validation loss). Otherwise one subplot per tag. |
There was a problem hiding this comment.
good catch, there are multiple combinations of overlay and passing axes that will fail here. I will push a fix.
| for name, vals in (("colors", colors), ("labels", labels), ("ylabel", ylabel)): | ||
| if vals is not None and len(vals) != len(tags): | ||
| raise ValueError( | ||
| f"`{name}` must have the same length as `tags` " | ||
| f"(got {len(vals)} vs {len(tags)})." | ||
| ) |
There was a problem hiding this comment.
on collecting vals lenghts first, I think it's fine to just do it on each iteration in the loop, it's only three times.
The other point is fixed with the recent commit.
3 participants
continuation of @Abraham-y work on #1814
inferencetotrainerto match our recent terminology; add deprecation warnings.fixes #1733