-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Abstract log_figure API that implements reasonable default for all lo… #6227
Changes from 23 commits
7b2d2d8
6f8bd9a
95b4d4e
dcdfbee
eca36b2
b0b67d7
a77028a
4424626
e11fc0c
32f67c6
6af3f1e
7f2407b
149d7a4
f545b94
289bd21
a1a3eed
1559b78
653bd36
855efd1
cf9218c
76bd3b2
87150dc
602b42c
a4175f2
ca0dd51
812d3c1
00b59a1
e775e76
82c6da9
fc86940
ec64bf8
126c0ec
f972410
5684258
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -49,6 +49,14 @@ | |
CometExperiment, CometExistingExperiment, CometOfflineExperiment = None, None, None | ||
API = None | ||
|
||
_MATPLOTLIB_AVAILABLE = _module_available("matplotlib") | ||
|
||
if _MATPLOTLIB_AVAILABLE: | ||
import matplotlib.pyplot as plt | ||
else: | ||
from pytorch_lightning.utilities.mock_types import matplotlib as _matplotlib | ||
plt = _matplotlib.pyplot | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it worth adding this just for the type annotation? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added this because someone suggested it above. Does not hurt, does it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. --> Needs review decision |
||
|
||
class CometLogger(LightningLoggerBase): | ||
r""" | ||
|
@@ -252,6 +260,13 @@ def log_metrics(self, metrics: Dict[str, Union[torch.Tensor, float]], step: Opti | |
metrics_without_epoch = self._add_prefix(metrics_without_epoch) | ||
self.experiment.log_metrics(metrics_without_epoch, step=step, epoch=epoch) | ||
|
||
@rank_zero_only | ||
def log_figure(self, name: str, figure: plt.figure, step: Optional[int] = None, close: bool = True) -> None: | ||
|
||
self.experiment.log_figure(figure_name=name, figure=figure, step=step) | ||
if close: | ||
plt.close(figure) | ||
|
||
def reset_experiment(self): | ||
self._experiment = None | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
# Copyright The PyTorch Lightning team. | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
|
||
class matplotlib: | ||
class pyplot: | ||
figure = None | ||
close = None |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
import numpy as np | ||
|
||
from pytorch_lightning.utilities import _module_available | ||
|
||
_MATPLOTLIB_AVAILABLE = _module_available("matplotlib") | ||
if _MATPLOTLIB_AVAILABLE: | ||
import matplotlib.pyplot as plt | ||
else: | ||
class plt: | ||
figure = None | ||
|
||
|
||
def dummy_figure() -> plt.figure: | ||
"""Dummy figure to test logging capability of figures for loggers.""" | ||
|
||
f = plt.figure() | ||
plt.plot(np.linspace(0., 1., 100), np.linspace(0., 10., 100) ** 2) | ||
|
||
return f |
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 think we should add additional
**kwargs
here.When the user knows they are using a particular logger that supports additinal arguments, we should allow it to be passed down.
for example, in one of the loggers you have a description, and this could be customized then by the user.
This would then still be logger agnostic, where loggers simply ignore unknown
**kwargs
when it's not applicable.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.
Funnily I had it including **kwargs two times, and changed it back and forth ...
I do not like it because:
LoggerCollection
unless one implements a more complex logic which then makes the whole implementation unnecessaryThere 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 also think
**kwargs
is really necessary here. Each logger can have many different argumentsI don't get this
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.
If you add **kwargs it will break LoggerCollection if you don't do anything. So what do you do if you have logger A that can take a certain argument that logger B in your collection does not? Ignore argument, raise?
Or add more complex logic that looks something like this
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.
--> Needs design decision