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

[ENH] Simplified saving kwargs #200

Closed
wants to merge 2 commits into from
Closed

[ENH] Simplified saving kwargs #200

wants to merge 2 commits into from

Conversation

ryanhammonds
Copy link
Contributor

@ryanhammonds ryanhammonds commented Mar 8, 2021

Addresses a point in #193. This reduces save_fig, file_name, and file_path when saving plots and file_name and file_path when saving reports, to only file_path. This is in-line with most packages (numpy, scipy, pandas, sklearn, etc) only having one argument to specify both file name and path when saving objects.

Here is a example with relative/abosolute paths with one file_path arg:

import os
import shutil
from fooof import FOOOF, FOOOFGroup
from fooof.sim import gen_group_power_spectra

# Simulate
freqs, powers = gen_group_power_spectra(10, (1, 50), [-2, 0], [10, 5, 5])

# Fit
fm = FOOOF(verbose=False)
fm.fit(freqs, powers[0])

fg = FOOOFGroup(verbose=False)
fg.fit(freqs, powers)

# Make a tmp directory
if os.path.isdir('tmpdir_'):
    shutil.rmtree('tmpdir_')
os.mkdir('tmpdir_')

# Relative Path
fm.plot(file_name='tmpdir_/plot')
fm.save(file_name='tmpdir_/results')
fm.save_report(file_name='tmpdir_/report')

fg.plot(file_name='tmpdir_/fg_plot')
fg.save(file_name='tmpdir_/fg_results', save_results=True)
fg.save_report(file_name='tmpdir_/fg_report')

assert os.path.isfile('tmpdir_/plot.png')
assert os.path.isfile('tmpdir_/results.json')
assert os.path.isfile('tmpdir_/report.pdf')
assert os.path.isfile('tmpdir_/fg_plot.png')
assert os.path.isfile('tmpdir_/fg_results.json')
assert os.path.isfile('tmpdir_/fg_report.pdf')

# Absolute Path
cwd = os.getcwd()

fm.plot(file_name=cwd + '/tmpdir_/plot')
fm.save(file_name=cwd + '/tmpdir_/results')
fm.save_report(file_name=cwd + '/tmpdir_/report')

fg.plot(file_name=cwd + '/tmpdir_/fg_plot')
fg.save(file_name=cwd + '/tmpdir_/fg_results', save_results=True)
fg.save_report(file_name=cwd + '/tmpdir_/fg_report')

assert os.path.isfile(cwd + '/tmpdir_/plot.png')
assert os.path.isfile(cwd + '/tmpdir_/results.json')
assert os.path.isfile(cwd + '/tmpdir_/report.pdf')
assert os.path.isfile(cwd + '/tmpdir_/fg_plot.png')
assert os.path.isfile(cwd + '/tmpdir_/fg_results.json')
assert os.path.isfile(cwd + '/tmpdir_/fg_report.pdf')

shutil.rmtree('tmpdir_')

@ryanhammonds ryanhammonds mentioned this pull request Apr 6, 2021
3 tasks
@TomDonoghue
Copy link
Member

Hey @ryanhammonds - like we talked about when we chatted, I'm not sure this is needed.

If an explicit file_name is passed, a file_path is not needed (but I see no harm / could be useful to be able to have it as a separate arg).

As a quick update, in #204, I have updated the savefig decorator to default to True if file_name is passed, meaning the explicit save_fig is no longer required to be passed (but can be left for people to use, to set False, if one wants to stop the saving). There is more discussion of this in equivalent change in NeuroDSP (neurodsp-tools/neurodsp#258). I think in the end this change allows for a simpler approach for saving, without needing to drop any arguments (keeping them if anyone wants to use them, and maintaining more backwards compatibility).

Overall then, I'm going to close this. If you want to chat more on any of this, let me know!

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

2 participants