Isbi em stacks crossvalidation argument #8

ArantxaCasanova · 2017-06-01T20:41:12Z

To be able to separate de data in folds and select a specific one for validation.

fvisin

Thank you for your contribution!!

Can you please make sure your code respects the PEP8 coding style rules?
The easiest way to do so is to install pep8 with pip install pep8 (with --local if you don't want to install it globally) and run pep8 sbi_em_stacks.py.py to check the code. Once you are done please commit the changes and I'll review the PR.
Thanks!

ArantxaCasanova · 2017-06-02T15:19:23Z

I already applied the coding style modifications.
If there is anything else, don't hesitate to ask.
Thank you!

fvisin

Thank you for the PR and sorry for keeping you waiting for so long!

Here is my review, there are a few modifications that I think will make the code easier to understand. Once you implement them we can merge it!

Thanks!

fvisin · 2017-07-05T10:32:05Z

dataset_loaders/images/isbi_em_stacks.py

-        whereas 15\% will be used for validation.
+        For example, if split=0.85, 85\% of the images will be used
+        for training, whereas 15\% will be used for validation.
+    crossval: int


int or None

Also please rename as crossval_nfolds

fvisin · 2017-07-05T10:33:00Z

dataset_loaders/images/isbi_em_stacks.py

+        for training, whereas 15\% will be used for validation.
+    crossval: int
+         If it is set to None, to cross-validation is used. An int specifying
+         in how many folds we want to split our data.


When None cross-validation is disabled. Else, represents the number of folds we data will be split into.

fvisin · 2017-07-05T10:34:28Z

dataset_loaders/images/isbi_em_stacks.py

@@ -52,20 +58,52 @@ class IsbiEmStacksDataset(ThreadedDataset):
        1: (255, 255, 255)}  # Membranes
    _mask_labels = {0: 'Non-membranes', 1: 'Membranes'}

-    def __init__(self, which_set='train', split=0.85, *args, **kwargs):
+    def __init__(self, which_set='train', split=0.60, crossval=5, fold=3, rand_perm=None,


The description of rand_perm is missing.

fvisin · 2017-07-05T10:35:07Z

dataset_loaders/images/isbi_em_stacks.py

+         in how many folds we want to split our data.
+    fold: int
+        An int specifying which fold we want. If fold=1, images from 0 to 5
+        will be used as validation. If fold=2, images from 6 to 11, and so on.


fold should be zero-based. Please change it to:

An int specifying which fold to use for validation. If fold=0, images from 0 to 5 will be used for validation. If fold=1, images from 6 to 11, and so on.

Rename to valid_fold

fvisin · 2017-07-05T10:36:17Z

dataset_loaders/images/isbi_em_stacks.py

-        elif self.which_set == "test":
-            self.start = 0
-            self.end = 30
+        self.middle_fold = False  # False by default


Please remove the comment, the code is already clear.

Please remove self.middle_fold altogether (see comment below)

fvisin · 2017-07-05T10:38:44Z

dataset_loaders/images/isbi_em_stacks.py

+            img_per_fold = int(30/crossval)
+            # start and end index for validation fold
+            start = (fold-1)*img_per_fold
+            end = fold*img_per_fold


make it (fold + 1) * img_per_fold
Edit: disregard, see comment below.

fvisin · 2017-07-05T10:40:54Z

dataset_loaders/images/isbi_em_stacks.py

-        For example, if split=0.85, 85\% of the images will be used for training,
-        whereas 15\% will be used for validation.
+        For example, if split=0.85, 85\% of the images will be used
+        for training, whereas 15\% will be used for validation.


Add "Will be ignored if crossval_nfolds is not None"

fvisin · 2017-07-05T12:15:56Z

dataset_loaders/images/isbi_em_stacks.py

+            elif self.which_set == "val":
+                self.start = start
+                self.end = end
+            elif self.which_set == "test":


I don't think 'test' makes sense in the cross-validation case. I suggest to raise a ValueError if which_set is 'test' here.

More generally, I suggest to replace L75-93 with the following approach:

if self.which_set == "train": self.start_1 = 0 self.end_1 = fold * img_per_fold self.start_2 = (fold+1) * img_per_fold self.end_2 = 30 elif self.which_set == "val": self.start_1 = fold * img_per_fold self.end_1 = self.start_2 = self.end_2 = (fold+1) * img_per_fold elif self.which_set == "test": raise ValueError('Cannot perform cross-validation on test.')

and then replace self.get_names with

return {'default': self.rand_indexes[self.start_1:self.end_1] + self.rand_indexes[self.start_2:self.end_2]}

fvisin · 2017-07-05T12:17:58Z

dataset_loaders/images/isbi_em_stacks.py

+            if rand_perm is not None:
+                self.rand_indexes=rand_perm
+            else:
+                self.rand_indexes=range(0,30)


self.rand_indexes = range(30)

fvisin · 2017-07-05T12:53:48Z

dataset_loaders/images/isbi_em_stacks.py

+            # if validation is a middle fold, concatenate separated train folds
+            return {'default': self.rand_indexes[range(0, self.start)+range(self.end, 30)].tolist()}
+        else:
+            return {'default': self.rand_indexes[range(self.start, self.end)].tolist()}


Replace with the suggested command (see comment above)

pep8speaks · 2017-07-05T16:54:55Z

Hello @ArantxaCasanova! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on July 05, 2017 at 16:58 Hours UTC

ArantxaCasanova · 2017-07-05T17:01:06Z

Thanks for the revision! I corrected what you suggested.

I still had to use "self.crossval = True" to control the "get_names()" function without making it too messy.
I also added the description for the "rand_perm" variable, I forgot to add it before.

Let me know what you think!

fvisin

Thank you for fixing the code and please excuse me for the embarrassingly long time it took me to review your new commit.

There are only two minor things I'd like you to fix, and then we can merge! Thank you very much for your contribution :)

fvisin · 2017-09-27T22:53:09Z

dataset_loaders/images/isbi_em_stacks.py

+            return {'default': (
+                                  self.rand_indices[self.start_1:self.end_1]
+                              ).tolist() + (
+                self.rand_indices[self.start_2:self.end_2]).tolist()}


The indentation makes it difficult to read the code.
This should respect the PEP8 while maintaining the readability of the code:

return {'default': self.rand_indices[self.start_1:self.end_1]).tolist() + ( self.rand_indices[self.start_2:self.end_2]).tolist()}

fvisin · 2017-09-27T22:56:40Z

dataset_loaders/images/isbi_em_stacks.py

-        self.middle_fold = False  # False by default
-        if crossval is not None:  # if cross-validation is used
+        if crossval_nfolds is not None:
+            self.crossval = True


I would prefer not to add this attribute to the class since the dataset objects are already a bit cluttered :)
I suggest to remove it and check for hasattr(self, 'rand_indices') in get_names()

fvisin · 2017-09-27T23:03:51Z

dataset_loaders/images/isbi_em_stacks.py

-        if self.middle_fold:
-            # if validation is a middle fold, concatenate separated train folds
-            return {'default': self.rand_indexes[range(0, self.start)+range(self.end, 30)].tolist()}
+        if self.crossval:


See comment above

ArantxaCasanova added 2 commits June 1, 2017 16:34

added cross-validation option

9e8fd5f

Change in description

ee585ff

fvisin suggested changes Jun 1, 2017

View reviewed changes

pep8 coding style changed

bf5e398

fvisin added the needs review label Jun 3, 2017

added random permutation for CV

60ec913

fvisin force-pushed the master branch 2 times, most recently from c6a8d70 to ff0bbfe Compare June 22, 2017 18:30

fvisin suggested changes Jul 5, 2017

View reviewed changes

fvisin added changes requested and removed needs review labels Jul 5, 2017

modified proposed changes

c3dffda

ArantxaCasanova added 2 commits July 5, 2017 12:56

small PEP8 modifications

dfa8d30

Merge branch 'master' into crossval_added

25abc82

fvisin added needs review and removed changes requested labels Sep 7, 2017

fvisin changed the title ~~Cross-validation option added~~ Isbi em stacks crossvalidation argument Sep 7, 2017

fvisin suggested changes Sep 27, 2017

View reviewed changes

fvisin added changes requested and removed needs review labels Sep 27, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Isbi em stacks crossvalidation argument #8

Isbi em stacks crossvalidation argument #8

ArantxaCasanova commented Jun 1, 2017

fvisin left a comment

ArantxaCasanova commented Jun 2, 2017

fvisin left a comment

fvisin Jul 5, 2017

fvisin Jul 5, 2017

fvisin Jul 5, 2017

fvisin Jul 5, 2017

fvisin Jul 5, 2017

fvisin Jul 5, 2017

fvisin Jul 5, 2017

fvisin Jul 5, 2017

fvisin Jul 5, 2017

fvisin Jul 5, 2017

fvisin Jul 5, 2017

fvisin Jul 5, 2017

fvisin Jul 5, 2017

fvisin Jul 5, 2017

pep8speaks commented Jul 5, 2017 •

edited

Loading

ArantxaCasanova commented Jul 5, 2017

fvisin left a comment

fvisin Sep 27, 2017

fvisin Sep 27, 2017

fvisin Sep 27, 2017

Isbi em stacks crossvalidation argument #8

Are you sure you want to change the base?

Isbi em stacks crossvalidation argument #8

Conversation

ArantxaCasanova commented Jun 1, 2017

fvisin left a comment

Choose a reason for hiding this comment

ArantxaCasanova commented Jun 2, 2017

fvisin left a 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

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

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

Choose a reason for hiding this comment

pep8speaks commented Jul 5, 2017 • edited Loading

Comment last updated on July 05, 2017 at 16:58 Hours UTC

ArantxaCasanova commented Jul 5, 2017

fvisin left a 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

pep8speaks commented Jul 5, 2017 •

edited

Loading