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
Plot functions for tensorboard data #593
Conversation
Codecov Report
@@ Coverage Diff @@
## main #593 +/- ##
==========================================
- Coverage 66.77% 66.02% -0.76%
==========================================
Files 67 68 +1
Lines 4199 4279 +80
==========================================
+ Hits 2804 2825 +21
- Misses 1395 1454 +59
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
Awesome, I think this is great! Left minor comments below, thanks a lot!
sbi/utils/tensorboard_output.py
Outdated
plot_kwargs: Dict[str, Any] = {}, | ||
) -> Tuple[Figure, Axes]: | ||
"""Plots tensorboard scalar events logged by the summary writer held by an | ||
inference object. |
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.
Nitpick: first line of docstring should be a single line PEP 257
sbi/utils/tensorboard_output.py
Outdated
log_dir: Union[str, Path], size_guidance=DEFAULT_SIZE_GUIDANCE | ||
) -> Dict[str, Dict[str, Dict[str, List[Any]]]]: | ||
"""Returns an exhaustive nested dictionary of all event data stored by tensorboards | ||
SummaryWriter. |
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.
Single line
sbi/utils/tensorboard_output.py
Outdated
3. tag type attribute: attribute of the event. | ||
|
||
Args: | ||
log_dir |
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.
Missing
sbi/utils/tensorboard_output.py
Outdated
|
||
Args: | ||
log_dir | ||
size_guidance: to avoid OOming. Defaults to tensorboards default size_guidance. |
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.
What's OOming?
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.
OOM = out of memory, because it loads most event data to memory at once
sbi/utils/tensorboard_output.py
Outdated
size_guidance: to avoid OOming. Defaults to tensorboards default size_guidance. | ||
|
||
Returns: | ||
Dict[str, Dict[str, Dict[str, Any]]] |
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.
type is already indicated above. Maybe add a one-line description?
Great, thanks a lot! One last request (sorry I didn't think of it before): could you add a test that checks the API? The test should run each of the three methods (SNPE, SNRE, SNLE) and run the plotting functions on the resulting _ = inference.append_simulations(theta, x).train(max_num_epochs=5) You can create a new file Thanks again! |
from matplotlib.axes import Axes | ||
|
||
|
||
def test_plot_summary(tmp_path): |
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.
To avoid code repetition, you could parameterize the test as done e.g. here
from matplotlib.figure import Figure | ||
from matplotlib.axes import Axes | ||
|
||
|
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'm confused...how does this test work? tmp_path
is required, so what is it set to?
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.
That’s some pytest magic, to avoid that files persist on the filesystem after a test. We can use it for any test writing sbi-logs.
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.
Ah, cool, thanks!
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.
Thanks a lot! I left two minor comments (questions), but feel free to ignore them and merge :)
* added vscode code-workspace to gitignore * Plot functions for tensorboard data #586 * fixed minor style issues * adding test for plot_summary
I added extraction for any tensorboard data and basic plotting for any scalar tensorboard data for diagnostics.
Usage examples
Plotting tensorboard data from inference object. The warning prompt is to support the user to start tensorboard for interactive visualization, as well as with hints on the valid tags.
Plotting tensorboard data from Path returned by
sbi.utils.list_all_logs(inference)
, to be able to iterate over log directories of past runs directly.Note: For possible future extensions, I wrote
_get_event_data_from_log_dir
to parse the tensorboard data into a convenient tabular format.