search

f-dangel / cockpit

Cockpit: A Practical Debugging Tool for Training Deep Neural Networks
MIT License
474 stars 24 forks source link

Plotter accepts savedir parameter but turns it into file path #8

Closed mseitzer closed 3 years ago

mseitzer commented 3 years ago

Description

Plotter has a savedir parameter for which the documentation says it specifies the directory where to save the plot to. However, the _save function interprets this parameter as a file path: https://github.com/f-dangel/cockpit/blob/937b3eac8d9fef7e6e6fbc9c91863dd08b1362b1/cockpit/plotter.py#L400-L405

This means plotting with savedir='/example/path' would result in a file /example/path__primary.png, where /example/path/__primary.png would be expected.

Besides, when choosing to not display the plot, I don't think __primary or __secondary should be added to the filename. From an API standpoint, it would probably be best to let the user flexibly choose the path and filename to save to (potentially adding the image extension).

fsschneider commented 3 years ago

Hi,

yes, this is indeed a bit of a mess, sorry for that.

Could you have a look at the savefig_handeling branch where I tried to address this? Specifically, I now added a savename argument to the CockpitPlotter.plot function, which lets you define the name, and now the savedir should really only describe the directory.

https://github.com/f-dangel/cockpit/blob/67257a68cd55e4c3dfa83875aaddab72cd1e02bb/cockpit/plotter.py#L61-L75

@f-dangel Could you also have a look over my changes?

mseitzer commented 3 years ago

Great, this looks more convenient. I would not insist on adding the __primary string though. If the plot is not displayed anywhere, this seems to be irrelevant for the filename.

fsschneider commented 3 years ago

Should be solved with #16.