Isbi em stacks crossvalidation argument by ArantxaCasanova · Pull Request #8 · fvisin/dataset_loaders

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

-        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

+        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

    _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

+         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

-        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

+            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

-        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

+            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

+            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

+            # 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

+                self.start_2 = (valid_fold + 1) * img_per_fold
+                self.end_2 = 30
            elif self.which_set == "val":
-                self.start = start


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

+    rand_perm: Numpy array or None
+        When a Numpy array with a random permutation is provided,
+        these random indices are used for the cross-validation.
+        When None, no random permutation is used.


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

-                    self.end = end
+                self.start_1 = 0
+                self.end_1 = valid_fold * img_per_fold
+                self.start_2 = (valid_fold + 1) * img_per_fold


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

Conversation

ArantxaCasanova commented Jun 1, 2017

Uh oh!

fvisin left a comment

Choose a reason for hiding this comment

Uh oh!

ArantxaCasanova commented Jun 2, 2017

Uh oh!

fvisin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pep8speaks commented Jul 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

ArantxaCasanova commented Jul 5, 2017

Uh oh!

fvisin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pep8speaks commented Jul 5, 2017 •

edited

Loading