Robodummy by Pranjal-sopho · Pull Request #57 · ilastik/tiktorch

Pranjal-sopho · 2019-06-13T12:02:05Z

No description provided.

update fork

mr_robot/.gitignore

FynnBe · 2019-06-13T12:06:51Z

mr_robot/mr_robot.py

+class BaseStrategy:
+
+    def __init__():
+        raise NotImplementedError


__init__ should not raise NotImplementedError. In fact, it is a good style to call super().__init__() in your derived class...

actually, this being work in progress, if you want to indicate that your BaseStrategy is not fully implemented yet, this is fine. (Calling super().__init__() in your derived class would still make sense)

mr_robot/utils.py

FynnBe · 2019-06-13T12:19:25Z

mr_robot/utils.py

+
+# create patches
+def tile_image2D(image_shape, tile_size):
+


it seems to me that image tiling could nicely be implemented for n dimensions. Maybe have a look at https://github.com/ilastik/lazyflow/blob/dfbb450989d4f790f5b19170383b777fb88be0e8/lazyflow/roi.py#L473 for some inspiration

FynnBe · 2019-06-17T09:24:48Z

mr_robot/mr_robot.py

@@ -0,0 +1,162 @@
+# import sys


we should avoid uncommented import statements (just remove this line)

FynnBe · 2019-06-17T10:40:30Z

mr_robot/mr_robot.py

+            base_config = yaml.load(f)
+
+        fut = self.new_server.load_model(base_config, model_file, binary_state, b"", ["cpu"])
+        print("model loaded")


[optional] use a logger, instead of print:

import logging logger = logging.getLogger(__name__) ... logger.info("model loaded")

more at https://docs.python.org/3/howto/logging.html

FynnBe · 2019-06-17T10:42:07Z

mr_robot/mr_robot.py

+        print("training resumed")
+
+    def predict(self):
+        self.ip = np.expand_dims(self.f['volume'][0,0:img_dim, 0:img_dim], axis = 0)


instead of taking the first slice [0, ...] and then expanding the resulting array, you should simplify to take a slice right away:
[0:1, ...]

m-novikov · 2019-06-17T09:29:28Z

mr_robot/mr_robot.py

+
+    def load_model(self):
+        # load the model
+        with open("state.nn", mode="rb") as f:


As we discussed these paths should be moved to robot config.

m-novikov · 2019-06-17T09:35:20Z

mr_robot/mr_robot.py

+        print("training resumed")
+
+    def predict(self):
+        self.ip = np.expand_dims(self.f['volume'][0,0:img_dim, 0:img_dim], axis = 0)


In general, variable names need some polishing. They should be descriptive and have a clear scope.

m-novikov · 2019-06-17T10:42:56Z

mr_robot/mr_robot.py

+
+    # run prediction
+    op = robo.predict()
+


Here I think algorithm should be read as follows:

# Step 1. Intialization robo = MrRobot('/home/user/config.yaml') # Here robot loads all required data robo.use_strategy(StrategyRandom()) # or even robo = MrRobot('/home/user/config.yaml', StrategyRandom) # Step 2. Start robo.start() # Start tiktorch server # Step 3. Prediction loop while robo.should_stop(): robo.predict() # def robo.predict # 1. labels? = self.strategy.get_next_patch(<relevant data>) # 2. self.update_training(labels, ...) # Step 4. Termination robo.terminate()

yes, I'd vote for

robo = MrRobot('/home/user/config.yaml', StrategyRandom)

FynnBe · 2019-06-17T10:45:51Z

mr_robot/mr_robot.py

+
+
+if __name__ == "__main__":
+


the following code should be inside of MrRobot. Currently you mirror parts of the tiktorch api in MrRobot (methods: resume, predict, add). This is fine for convenience, etc, but in it's core MrRobot should implement the way of running a 'user simulation'

FynnBe · 2019-06-17T10:47:42Z

mr_robot/mr_robot.py

+    def base_loss(self, patch, label):
+        label = label[0][0]
+        patch = patch[0][0]
+        result = mean_squared_error(label, patch)  # CHECK THIS


the criterion should be configurable

FynnBe · 2019-06-17T10:49:02Z

mr_robot/mr_robot.py

+
+    def run(self):
+        idx = tile_image(self.op.shape, patch_size)
+        label = np.expand_dims(self.f['volumes/labels/neuron_ids'][0,0:img_dim,0:img_dim], axis=0)


same indexing as in predict method

FynnBe · 2019-06-17T10:54:13Z

mr_robot/mr_robot.py

+    def __init__(self, file, op):
+        super().__init__(file,op)
+
+    def run(self):


I would prefer the robot class to perform the 'run', not the strategy. The strategy should effectively implement a sampling strategy. I see this analog to the pytorch sampler.
We might even be able to use the pytorch dataset and the pytorch dataloader for our purposes (and then implement our strategy as a 'Sampler'

FynnBe · 2019-06-19T07:00:33Z

mr_robot/mr_robot.py

+        with open(path_to_config_file, mode="r") as f:
+            self.base_config = yaml.load(f)
+
+        self.max_robo_iterations = self.base_config['max_robo_iterations']


including _robo_ in this variable name seems redundant, considering we are in the MrRobot class

FynnBe · 2019-06-19T07:02:31Z

mr_robot/mr_robot.py

+            self.base_config = yaml.load(f)
+
+        self.max_robo_iterations = self.base_config['max_robo_iterations']
+        self.counter = 0


counter as a property name is a bit confusing here (it is not obvious what's being counted)
I would suggest

self.iterations_max

self.iterations_done

if that is what you intend

FynnBe · 2019-06-19T07:03:38Z

mr_robot/mr_robot.py

+        #with open(base_config['cremi_data_dir'], mode="rb") as f:
+        #    binary_state = f.read()
+
+        archive = zipfile.ZipFile(self.base_config['cremi_dir']['path_to_zip'], 'r')


you should run black on your code (this will convert ' to " where possible)

FynnBe · 2019-06-19T07:05:40Z

mr_robot/mr_robot.py

+            self.add(idx)
+
+    def add(self, idx):
+        file = z5py.File(self.base_config["cremi_data"])


no need to open this file every time add is called. This should be in __init__. use self.file here

FynnBe · 2019-06-19T07:08:22Z

mr_robot/mr_robot.py

+            self.add(idx)
+
+    def add(self, idx):
+        file = z5py.File(self.base_config["cremi_data"])


"cremi_data" should instead be something like "raw_data_path" and "label_data_path" (1. cremi is just an example. 2. raw data and label data are not necessarily in the same file)

FynnBe · 2019-06-19T07:09:13Z

mr_robot/mr_robot.py

+        file = z5py.File(self.base_config["cremi_data"])
+        labels = file["cremi_path_to_labelled"][0:1, 0:img_dim, 0:img_dim]
+
+        new_ip = self.ip.as_numpy()[idx[0]:idx[1], idx[2]:idx[3], idx[4]:idx[5]].astype(float)


you should not hardcode that the data is 3 dimensional, use tuples to index instead

FynnBe · 2019-06-19T07:12:20Z

mr_robot/mr_robot.py

+        return result
+
+    def base_patch(self, loss_fn, op):
+        idx = tile_image(op.shape, patch_size)


it would be great if you could add some doc strings to communicate what your methods (and classes) are for

hint: https://www.python.org/dev/peps/pep-0008/#documentation-strings

also: no need to call this method base_patch. naming it patch and calling super().patch() in a derived class works as well (even when you overwrite patch in the derived class, that's what the super(), resolves for you)
some more 'how-to-inherit' here: https://www.python.org/dev/peps/pep-0008/#designing-for-inheritance

FynnBe · 2019-07-12T09:21:46Z

.gitignore

-.py~
+.py~
+*.nn
+*.hdf


there is no need to ignore .nn and .hdf files (as there are none in the repo). Pls remove

FynnBe · 2019-07-12T09:25:19Z

mr_robot/mr_robot.py

+    """ The robot class runs predictins on the model, and feeds the
+    worst performing patch back for training. The order in which patches
+    are feed back is determined by the 'strategy'. The robot can change
+    strategies as training progresses.


we decided that changing strategy is a strategy of its own...

FynnBe · 2019-07-12T09:34:01Z

mr_robot/mr_robot.py

+
+        self.iterations_max = self.base_config.pop("max_robo_iterations")
+        self.iterations_done = 0
+        self.tensorboard_writer = SummaryWriter(logdir="/home/psharma/psharma/repos/tiktorch/tests/robot/robo_logs")


do not hard code 'personal' paths, etc...
suggestion:
get absolute path of mr_robot.py and deduct the absolute path to mr_robot folder:

mr_robot_folder = os.path.dirname(os.path.abspath(__file__))

add log folder to it:

logdir=os.path.join(mr_robot_folder, "logs")

FynnBe · 2019-07-12T09:41:31Z

mr_robot/utils.py

+        block_list[i] = tuple(block_list[i])
+
+    return block_list
+"""


delete if this is no longer needed

FynnBe · 2019-07-12T09:46:53Z

mr_robot/mr_robot.py

+        # cleaning dictionary before passing to tiktorch
+        self.base_config.pop("model_dir")
+
+        self.new_server.load_model(self.base_config, model, binary_state, b"", ["gpu:4"])


do not hard code use of a specific gpu, use environment variables

reatain fwd pass ids

retain ids from forward pass

…t compilation removed

Pranjal-sopho added 7 commits June 11, 2019 14:18

robo user first commit

5b8c573

Merge pull request #1 from ilastik/master

04e3fd9

update fork

some changes

73e4c47

Merge remote-tracking branch 'origin/master' into robodummy

2dec996

structural changes

3d66cbc

some structural changes

7bf8f81

deleted unnecessary files

d157d9d

FynnBe reviewed Jun 13, 2019

View reviewed changes

mr_robot/.gitignore Outdated Show resolved Hide resolved

FynnBe reviewed Jun 13, 2019

View reviewed changes

mr_robot/utils.py Show resolved Hide resolved

FynnBe reviewed Jun 13, 2019

View reviewed changes

Pranjal-sopho added 2 commits June 13, 2019 15:57

changed tiling to generic+ apply black

39fb936

switched to n5 for better mem mang.

52d0114

FynnBe reviewed Jun 17, 2019

View reviewed changes

mr_robot/mr_robot.py Outdated

@@ -0,0 +1,162 @@

# import sys

Copy link

Member

FynnBe Jun 17, 2019

Choose a reason for hiding this comment

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

we should avoid uncommented import statements (just remove this line)

FynnBe reviewed Jun 17, 2019

View reviewed changes

m-novikov reviewed Jun 17, 2019

View reviewed changes

FynnBe reviewed Jun 17, 2019

View reviewed changes

Pranjal-sopho added 2 commits June 18, 2019 18:04

incorporated suggestions

83316c2

added test folder

8814e98

FynnBe reviewed Jun 19, 2019

View reviewed changes

Pranjal-sopho added 3 commits July 8, 2019 14:34

tensorboard errors fixed

a9cfecd

new strategy added

fd1eb97

sparse annotation strategies added

cba7191

FynnBe reviewed Jul 12, 2019

View reviewed changes

mr_robot/utils.py Outdated

block_list[i] = tuple(block_list[i])

return block_list

"""

Copy link

Member

FynnBe Jul 12, 2019

Choose a reason for hiding this comment

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

delete if this is no longer needed

FynnBe reviewed Jul 12, 2019

View reviewed changes

Pranjal-sopho and others added 22 commits July 17, 2019 15:51

video labelling strategies added

f265faf

class annotator added

fcce3f2

all bugs fixed

4daa134

apply black

6be7a82

Merge pull request #3 from ilastik/master

f38692a

reatain fwd pass ids

new commit

27c4f43

added gitignore

d1a1cdc

Merge branch 'master' into robodummy

064c27c

retain ids from forward pass

testing..

4c6d9cb

Update environemnt file to include MrRobot deps

3940c55

Add __init__ and __main__

5da186d

Make __main__ work

f45d619

Fix confusion_matrix nans

3db31e9

Fixes to strategies

79a77da

Fix unstopabble predictions of inferno trainer

6bd9d7e

Add train_for method to tiktorch server

055d08f

Use train_for in mr_robot

3eec9c8

training problems fixed (temporarily)

8e907e3

training problems fixed (temporarily)

c6ddc1d

training problems fixed (temporarily)

617a4df

updating with code used for result prep

39a674d

strategy params passed generalized and hardcodings for done for resul…

9050f93

…t compilation removed

-              .py~

                
                    No newline at end of file
+              .py~
+              *.nn
+              *.hdf

Conversation

Pranjal-sopho commented Jun 13, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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!

FynnBe Jul 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

FynnBe Jul 12, 2019 •

edited

Loading