API: Datahandler now functions as a class #156

swkeemink · 2021-06-03T17:17:27Z

This involves quite a lot of changes.
This is classed as an API change as anyone using custom dataloaders in
our previous way will either have to stay with an older version of
FISSA, or migrate their datahandlers to this new method.

The tutorial notebook now extensively explains this at the end.

Another important change this brings is that for multiprocessing the datahandler class instance is now passed into the function for multiprocessing instead of defined as a global library, which will help with solving #147.

scottclowe

Changes requested. Main thing is I want to see the two DataHandler classes in the same file. When that's done, the unit tests should also be refactored into the same file. I think it would be better to use an abstract class, though this is not strictly necessary.

I've not read the notebook changes yet because they don't render on GitHub. Presumably the notebook changes are only to do with using a custom datahandler?

scottclowe · 2021-06-04T10:21:03Z

fissa/tests/test_datahandler.py

 from .. import roitools
-
+datahandler = DataHandler()


I don't like this. datahandler is the name of the module and DataHandler the name of the class. Should not make the name of the instance of DataHandler clash with the name of the module. Need to use a different name for the instance, such as

Suggested change

datahandler = DataHandler()

dh = DataHandler()

(which obviously has implications for later code in the file which needs datahandler changed to dh)

Maybe we should consider renaming the module instead though.

I've not read the notebook changes yet because they don't render on GitHub. Presumably the notebook changes are only to do with using a custom datahandler?

Yes. With an explanation of how to use it.

Maybe we should consider renaming the module instead though.
The case of datahandler = DataHandler() is actually consistent with how use core.Experiment() class (we call exp=fissa.Experiment() or experiment=fissa.Experiment() a few times in the examples). But I agree it is not the best naming. Now I import DataHandler from datahandler, and then make an DataHandler object called datahandler. Yuk.

We could rename the module to extraction (following Suite2p) and keep the class-name, as well as use your dh suggestion, so it becomes

Suggested change

datahandler = DataHandler()

from ..extraction import DataHandler()

dh = DataHandler()

scottclowe · 2021-06-04T10:21:12Z

fissa/tests/test_datahandler_framebyframe.py

 from .. import roitools
-
+datahandler = DataHandler()


As for the other file.

scottclowe · 2021-06-04T10:30:21Z

fissa/datahandler_framebyframe.py



-def image2array(image):
-    """Open a given image file as a PIL.Image instance.
+class DataHandler(datahandler.DataHandler):


Great to see this being done as a subclass. But I'd actually like to see the two data handlers put in the same file.

class DataHandler(): # existing code as in other file class DataHandlerFrameByFrame(DataHandler): # code as you have in this file

That would be much tidier. Also, you've got this weird thing at the moment where you can do

import datahandler_framebyframe x = datahandler_framebyframe.DataHandler() y = datahandler_framebyframe.datahandler.DataHandler() x.__class__ == y.__class__ # Returns False and x and y have different behaviours # Not behaviour that a user of this API would expect.

Also, I think it might be better coding practice to use an abstract class (DataHandlerAbstract) and have both DataHandler and DataHandlerFrameByFrame inherit from the abstract class instead of having DataHandlerFrameByFrame inherit from DataHandler. Because they are more like siblings than parent/child. Also then we expect custom made data handler objects to inherit from DataHandlerAbstract, but they don't have to inherit from DataHandler.

You should write DataHandlerAbstract to have all the methods used it DataHandler defined as raise NotImplementedError(). Any methods which are common to all data handlers can be implemented for real and then the children don't have to overwrite those methods.

Agreed. Then we can also merge the datahandler test files.

About an abstract class -- I will implement it (and it will have only NotImplementedErrors, because all the functions are different for framebyframe!), but might not use it in the tutorial. It makes a lot more sense for most users to only edit one or two of the functions used in the standard DataHandler. I'll just make a comment for advanced users that they can define an entirely new handler from DataHandlerAbstract.

scottclowe · 2021-06-07T08:39:28Z

Also, commit f9efaff is off-topic and should be in a different PR concerning verbosity changes. Sorry I missed that before!

swkeemink · 2021-06-07T08:56:24Z

Also, commit f9efaff is off-topic and should be in a different PR concerning verbosity changes. Sorry I missed that before!

Actually it does fit here because it fixes an error I introduced myself earlier in this PR.>.>

scottclowe · 2021-06-07T08:59:33Z

Also, commit f9efaff is off-topic and should be in a different PR concerning verbosity changes. Sorry I missed that before!

Actually it does fit here because it fixes an error I introduced myself earlier in this PR.>.>

Oh, I see. I missed it because it's not part of the PR's diff. Could you rebase and squash f9efaff into the commit that did the thing that it is undoing?

swkeemink · 2021-06-07T11:59:15Z

I updated this PR. The datahandler module has been renamed to the extraction module. In the end I opted for datahandler = DataHandler() for the class declarations though, as dh = DataHandler() was much less intuitive to read the code with generally. Personally I quite like it like this and it is consistent with how we declare FISSA experiments, but happy to hear name change suggestions.

Some possibilities
datahandler = DataReader()

datahandler = ExtractTool()

swkeemink · 2021-06-07T12:12:14Z

Aha! I can also rename the current DataHandler() class to DataHandlerTifffile(). This is already like that for the pillow data loader and the custom data loader after all.

swkeemink · 2021-06-07T12:20:59Z

Aha! I can also rename the current DataHandler() class to DataHandlerTifffile(). This is already like that for the pillow data loader and the custom data loader after all.

This is now done. I'm happy with this to be merged into master and then we can do the other PRs.

This involves quite a lot of changes. This is classed as an API change as anyone using custom dataloaders in our previous way will either have to stay with an older version of FISSA, or migrate their datahandlers to this new method. I will describe this in detail in the tutorial notebook that is to follow in another commit.

fissa/core.py

scottclowe · 2021-06-07T14:10:58Z

fissa/tests/test_core.py

+        self.datahandler = DataHandlerTifffile()
+


This isn't something that's a common dependency across tests, so it should not be defined here. If there's something wrong with this line of code, it should only be breaking the one or two tests that use a datahandler and not every test that's part of this class.

Suggested change

self.datahandler = DataHandlerTifffile()

scottclowe · 2021-06-07T15:53:49Z

fissa/tests/test_extraction.py

+        self.assert_equal(actual, self.expected)
+
+
+class TestRois2Masks(BaseTestCase):


Suggested change

class TestRois2Masks(BaseTestCase):

class TestRois2MasksTifffile(BaseTestCase):

scottclowe · 2021-06-07T15:56:28Z

fissa/tests/test_extraction.py

+from ..extraction import DataHandlerTifffile,  DataHandlerPillow
+from .. import roitools
+
+class TestImage2Array(BaseTestCase):


Need to stop these classes colliding with the same name!

Suggested change

class TestImage2Array(BaseTestCase):

class TestImage2ArrayTifffile(BaseTestCase):

scottclowe · 2021-06-07T15:57:15Z

fissa/tests/test_extraction.py

+        os.remove('test.tif')
+
+
+class TestRois2Masks(BaseTestCase):


Suggested change

class TestRois2Masks(BaseTestCase):

class TestRois2MasksPillow(BaseTestCase):

scottclowe · 2021-06-07T15:57:26Z

fissa/tests/test_extraction.py

+            self.datahandler.rois2masks(polys3d, self.data)
+
+
+class TestImage2Array(BaseTestCase):


Suggested change

class TestImage2Array(BaseTestCase):

class TestImage2ArrayPillow(BaseTestCase):

fissa/extraction.py

Co-authored-by: Scott Lowe <[email protected]>

…ahandler loading Co-authored-by: Scott Lowe <[email protected]>

Co-authored-by: Scott Lowe <[email protected]>

codecov · 2021-06-09T09:11:26Z

Codecov Report

Merging #156 (53af2ab) into master (50a0ca5) will increase coverage by 0.28%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #156      +/-   ##
==========================================
+ Coverage   92.68%   92.97%   +0.28%     
==========================================
  Files           9        8       -1     
  Lines         779      811      +32     
  Branches      158      161       +3     
==========================================
+ Hits          722      754      +32     
  Misses         29       29              
  Partials       28       28

Flag	Coverage Δ
unittests	`92.84% <100.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
fissa/core.py	`96.16% <100.00%> (+0.25%)`	⬆️
fissa/extraction.py	`100.00% <100.00%> (ø)`

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 50a0ca5...53af2ab. Read the comment docs.

swkeemink mentioned this pull request Jun 3, 2021

Make a datahandler class #16

Closed

scottclowe added the vX.Y.0 New Minor Release This should be included in a minor release label Jun 3, 2021

scottclowe self-requested a review June 3, 2021 17:29

scottclowe requested changes Jun 4, 2021

View reviewed changes

scottclowe mentioned this pull request Jun 7, 2021

Update the tif reading testing and implementation #170

Merged

swkeemink mentioned this pull request Jun 7, 2021

[WIP] RF: Move rois2masks from datahandler to roitools #100

Closed

swkeemink force-pushed the datahandler_class branch from fb0cc12 to a1d0c45 Compare June 7, 2021 15:21

swkeemink added 13 commits June 7, 2021 17:24

JNB: updated the tutorial notebook for the new datahandler method

715405d

TST: updated the relevant tests for the new datahandler method

0fa3466

JNB: cleaned up tutorial notebook a bit

95eee04

RF: import pillow in basic datahandler file

f384abf

ENH: defined an abstract datahandler class

367afed

STY: whitespace update

5aaca93

RF: added and renamed framebyframe datahandler to basic datahandler file

7a48643

API: renamed datahandler to extraction

7863c42

RF: removed datahandler_framebyframe

7825ab0

TST: API: updated tests for datahandler>extraction rename

d58fccc

API: updated core for datahandler>extraction rename

42c31b6

TST: RF: renamed datahandler test for extraction name change

5ed5b5f

BUG: added missing import

9781de9

swkeemink force-pushed the datahandler_class branch from a1d0c45 to f55c8f7 Compare June 7, 2021 15:28

scottclowe requested changes Jun 7, 2021

View reviewed changes

RF: merged tests for standard and framebyframe loading

53c95c0

swkeemink and others added 14 commits June 7, 2021 18:08

JNB: updated the basic usage notebook for datahandler changes

c858f14

API: renamed default datahandler class to DataHandlerTifffile

7c67cef

TST:API: updated the tests for the default datahandler rename

4c8ccfa

DOC: updated the basic notebook for the default dataloader name change

8a13326

DOC: change TBT to type

8e0dc77

TST:RF: load datahandler inside test

ca32d5f

RF: import extraction module on top

8f029cb

Co-authored-by: Scott Lowe <[email protected]>

API: changed datahandler_custom to just datahandler, and simplify dat…

c36db10

…ahandler loading Co-authored-by: Scott Lowe <[email protected]>

DOC: clarified class object used

d33bf18

Co-authored-by: Scott Lowe <[email protected]>

DOC: clarified and prettified low_memory mode explanation

a6c695d

Co-authored-by: Scott Lowe <[email protected]>

TST:RF: split test_polys_not_2d() into two tests

7edd9d8

Co-authored-by: Scott Lowe <[email protected]>

STY: whitespace update

1d0b697

Co-authored-by: Scott Lowe <[email protected]>

DOC: extraction docstring update

fbb04ef

Co-authored-by: Scott Lowe <[email protected]>

TST:RF: inherit Rois2MaskTifffile class for pillow testing

4e952e0

Co-authored-by: Scott Lowe <[email protected]>

swkeemink force-pushed the datahandler_class branch from 7f5a4bf to 4e952e0 Compare June 7, 2021 17:34

scottclowe added 4 commits June 8, 2021 16:42

JNB: Improvements to extraction/datahandler description

351d72c

DOC: Improve docstrings

3059d9b

MNT: Change to staticmethod for datahandler methods

e878d23

CI:Travis: Allow suite2p to fail

53af2ab

scottclowe merged commit cbfffd2 into rochefort-lab:master Jun 9, 2021

swkeemink mentioned this pull request Jun 16, 2021

Update Suite2P implementation #150

Closed

swkeemink deleted the datahandler_class branch June 29, 2021 15:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API: Datahandler now functions as a class #156

API: Datahandler now functions as a class #156

swkeemink commented Jun 3, 2021

scottclowe left a comment

scottclowe Jun 4, 2021

swkeemink Jun 7, 2021 •

edited

Loading

swkeemink Jun 7, 2021

scottclowe Jun 4, 2021

scottclowe Jun 4, 2021 •

edited

Loading

swkeemink Jun 7, 2021

swkeemink Jun 7, 2021

scottclowe commented Jun 7, 2021

swkeemink commented Jun 7, 2021

scottclowe commented Jun 7, 2021

swkeemink commented Jun 7, 2021

swkeemink commented Jun 7, 2021

swkeemink commented Jun 7, 2021

scottclowe Jun 7, 2021

scottclowe Jun 7, 2021

scottclowe Jun 7, 2021

scottclowe Jun 7, 2021

scottclowe Jun 7, 2021

codecov bot commented Jun 9, 2021 •

edited

Loading

	datahandler = DataHandler()
	from ..extraction import DataHandler()
	dh = DataHandler()

		self.assert_equal(actual, self.expected)


		class TestRois2Masks(BaseTestCase):

	class TestRois2Masks(BaseTestCase):
	class TestRois2MasksTifffile(BaseTestCase):

	class TestImage2Array(BaseTestCase):
	class TestImage2ArrayTifffile(BaseTestCase):

	class TestRois2Masks(BaseTestCase):
	class TestRois2MasksPillow(BaseTestCase):

		self.datahandler.rois2masks(polys3d, self.data)


		class TestImage2Array(BaseTestCase):

	class TestImage2Array(BaseTestCase):
	class TestImage2ArrayPillow(BaseTestCase):

API: Datahandler now functions as a class #156

API: Datahandler now functions as a class #156

Conversation

swkeemink commented Jun 3, 2021

scottclowe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swkeemink Jun 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottclowe Jun 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottclowe commented Jun 7, 2021

swkeemink commented Jun 7, 2021

scottclowe commented Jun 7, 2021

swkeemink commented Jun 7, 2021

swkeemink commented Jun 7, 2021

swkeemink commented Jun 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 9, 2021 • edited Loading

Codecov Report

swkeemink Jun 7, 2021 •

edited

Loading

scottclowe Jun 4, 2021 •

edited

Loading

codecov bot commented Jun 9, 2021 •

edited

Loading