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

Add the ability to save the plot #23

Closed
wants to merge 2 commits into from

Conversation

joanna-janos
Copy link

No description provided.

@NaleRaphael
Copy link
Contributor

NaleRaphael commented Mar 6, 2020

Hi @joanna-janos , thanks for your contribution.

Before @davidtvs reviewes this PR, you may note that this package still needs to support Python 2.7. However, the builtin module pathlib you used was introduced since Python 3.4.
Therefore, you might need to change the code to make it compatible with Python 2.7.

@joanna-janos
Copy link
Author

Hi @NaleRaphael,

sorry for that, I have changed code to make it compatible with python 2.7 (use os instead of pathlib)

Copy link
Contributor

@NaleRaphael NaleRaphael left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't be sorry, this is a nice improvement!
Here is a change request, could you please check that?

parent_directory.mkdir(parents=True, exist_ok=True)
print(f'Saving plot under "{path}"')
plt.savefig(path)
os.mkdir(parent_directory)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to use os.makedirs instead of os.mkdir for multi-layer directory.
Otherwise, you have to use try...except to handle FileNotFoundError here.
(In my opinion, os.makedirs is better for user experience)

@davidtvs
Copy link
Owner

davidtvs commented Mar 7, 2020

I would prefer to do this by returning the matplotlib figure and axes. The user can then choose if he wants to save and how he wants to do it. This way there's no additional parameter, no extra code on our side, and it's also more flexible for the user (plot can be manipulated and then saved in whatever way the user wants)

@NaleRaphael
Copy link
Contributor

NaleRaphael commented Mar 7, 2020

@davidtvs You're right, this approach is much more flexible!
BTW, it seems that plt.gcf() is another approach for user to get the handler of latest generated figure.
But it would be much more difficult than just returning the handler to user.

@davidtvs
Copy link
Owner

davidtvs commented Mar 9, 2020

@joanna-janos thanks for bringing this to our attention. I'm closing this PR but feel free to open a new one that returns the figure and the axis from plot

@davidtvs davidtvs closed this Mar 9, 2020
davidtvs added a commit that referenced this pull request Mar 22, 2020
@davidtvs
Copy link
Owner

FYI, the commit 70b6c1f implements the functionality that I described above.

@joanna-janos to save the figure you can now do:

# Create the figure and axes object outside and pass the axes to lr_finder.plot
fig, ax = plt.subplots()
lr_finder.plot(ax=ax)

# ax can be further manipulated by the user after lr_finder.plot

fig.savefig("test.png")

Note that when you pass the Axes object to lr_finder.plot() the figure is not shown (no plt.show()) to avoid destroying the figure before you can save it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants