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 option to not save generated data #86

Closed
wants to merge 7 commits into from
Closed

Add option to not save generated data #86

wants to merge 7 commits into from

Conversation

swkeemink
Copy link
Member

When defining an experiment it is now possible to define the format in which data is saved with the data_format option. At the moment this can only be 'npy' or None. If it is 'npy', the behavior will be as before. If it is set to None, no data will be saved. This is useful for users that don't need to store any of the data directly on the hard drive, and just want to define how and where to use the data in their own workflows.

I have also laid the basic groundwork for other formats than npy, and am working on saving everything in hdf5 files.

Note that there is currently a bug with our use of numpy.load, see #85.

@swkeemink swkeemink requested a review from scottclowe March 6, 2020 17:58
@swkeemink swkeemink changed the title Adding the option to change the data format Adding an option to change the data format Mar 6, 2020
@scottclowe scottclowe added vX.Y.0 New Minor Release This should be included in a minor release enhancement labels Mar 8, 2020
@scottclowe
Copy link
Member

If I understand correctly, you're intending to merge this as-is with the option of not saving added, and HDF5 support will be in a different PR?

@swkeemink
Copy link
Member Author

Correct!

fissa/core.py Show resolved Hide resolved
examples/Basic usage.ipynb Show resolved Hide resolved
Comment on lines -131 to +132
lowmemory_mode=False, datahandler_custom=None):
lowmemory_mode=False, datahandler_custom=None,
data_format='npy'):
Copy link
Member

Choose a reason for hiding this comment

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

Better code style is to have the ): on its own line, so you don't get this diff where a comma and a new line are inserted later on (which happened here).

Suggested change
lowmemory_mode=False, datahandler_custom=None):
lowmemory_mode=False, datahandler_custom=None,
data_format='npy'):
lowmemory_mode=False, datahandler_custom=None,
data_format='npy',
):

Copy link
Member Author

Choose a reason for hiding this comment

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

Very good point, I will change this.

@@ -196,6 +197,9 @@ def __init__(self, images, rois, folder, nRegions=4,
A custom datahandler for handling ROIs and calcium data can
be given here. See datahandler.py (the default handler) for
an example.
data_format : string or None, optional
How FISSA generated data will be saved.
Can be 'npy' (default) or None. In the future will also support 'hdf5'.
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't add comments about prospective future features to docstrings.

Copy link
Member

Choose a reason for hiding this comment

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

I think you need to specify the behaviour better. At the moment it isn't clear what happens if None is provided.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will clarify this in the docstring.

fissa/core.py Outdated Show resolved Hide resolved
fissa/core.py Outdated Show resolved Hide resolved
Comment on lines +200 to +202
data_format : string or None, optional
How FISSA generated data will be saved.
Can be 'npy' (default) or None. In the future will also support 'hdf5'.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
data_format : string or None, optional
How FISSA generated data will be saved.
Can be 'npy' (default) or None. In the future will also support 'hdf5'.
data_format : 'npy' or None, optional
The format in which data generated by FISSA will be saved.
If this is `'npy'`, data will be saved in the numpy format using
`numpy.save`. If this is `None`, the data will not be saved to
a cache file and will have to be generated again if the same
data is separated again. Default is `'npy'`.

fissa/core.py Outdated Show resolved Hide resolved
Comment on lines +292 to +298
# try to load data from filename
if not redo:
try:
nCell, raw, roi_polys = np.load(fname)
print('Reloading previously prepared data...')
except BaseException:
redo = True
Copy link
Member

Choose a reason for hiding this comment

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

Remove superfluous nesting.

Suggested change
# try to load data from filename
if not redo:
try:
nCell, raw, roi_polys = np.load(fname)
print('Reloading previously prepared data...')
except BaseException:
redo = True
# try to load data from filename
if not redo:
try:
nCell, raw, roi_polys = np.load(fname)
print('Reloading previously prepared data...')
except BaseException:
redo = True

fissa/core.py Outdated Show resolved Hide resolved
@scottclowe scottclowe changed the title Adding an option to change the data format Add option to not save generated data Mar 9, 2020
@swkeemink
Copy link
Member Author

Besides the above changes (which I generally approve of) we should also expand the tests to avoid reducing code coverage as it is now.

@codecov
Copy link

codecov bot commented Mar 16, 2020

Codecov Report

Merging #86 into master will decrease coverage by 1.33%.
The diff coverage is 58.62%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #86      +/-   ##
==========================================
- Coverage   79.27%   77.94%   -1.34%     
==========================================
  Files           9        9              
  Lines         666      680      +14     
  Branches      129      136       +7     
==========================================
+ Hits          528      530       +2     
- Misses         94       99       +5     
- Partials       44       51       +7
Impacted Files Coverage Δ
fissa/core.py 85.52% <58.62%> (-4.67%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 50f148c...0a41db9. Read the comment docs.

swkeemink and others added 3 commits March 16, 2020 12:05
Co-Authored-By: Scott Lowe <[email protected]>
Co-Authored-By: Scott Lowe <[email protected]>
@scottclowe
Copy link
Member

Closed in favour of #129, which handles the ability to run fissa without saving to a cache more succinctly. @swkeemink please make a new PR for saving to HDF5 format.

@scottclowe scottclowe closed this May 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement vX.Y.0 New Minor Release This should be included in a minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants