Skip to content
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

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

Closed
mseitzer opened this issue Jul 20, 2021 · 3 comments
Closed

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

mseitzer opened this issue Jul 20, 2021 · 3 comments
Assignees
Labels
🆕 Status: New New Issue 🐛 Type: Bug Something isn't working
Milestone

Comments

@mseitzer
Copy link

mseitzer commented Jul 20, 2021

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:

cockpit/cockpit/plotter.py

Lines 400 to 405 in 937b3ea

file_path = (
os.path.splitext(logpath)[0]
+ f"__{screen}"
+ savename_append
+ self.save_format
)

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).

@mseitzer mseitzer added 🆕 Status: New New Issue 🐛 Type: Bug Something isn't working labels Jul 20, 2021
@fsschneider
Copy link
Collaborator

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.

def plot(
self,
source,
show_plot=True,
block=False,
save_plot=False,
savedir=None,
savename="cockpit",
savename_append=None,
savefig_kwargs=None,
show_log_iter=False,
discard=None,
plot_title=None,
debug=False,
):

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

@mseitzer
Copy link
Author

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
Copy link
Collaborator

Should be solved with #16.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 Status: New New Issue 🐛 Type: Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants