Reorganisation Fixes #120

pushkalkatara · 2019-08-18T01:20:56Z

Checked all examples and fixed a few issues.

edgeml_pytorch/trainer/bonsaiTrainer.py

harsha-simhadri · 2019-08-18T09:07:36Z

examples/tf/EMI-RNN/00_emi_lstm_example.ipynb

-      "y_train shape is: (6294, 6, 6)\n",
-      "x_test shape is: (1058, 6, 48, 9)\n",
-      "y_test shape is: (1058, 6, 6)\n"
+      "x_train shape is: (6409, 6, 48, 9)\n",


@pushkalkatara Why did the x_train shape change?

@harsha-simhadri Due to data generation script, the shapes on each processing is varying. i.e

Processing data Extracting features ('subinstanceLen', 48) ('subinstanceStride', 16) ('sourceDir', '/home/pushkalkatara/mr/EdgeML/examples/tf/EMI-RNN/HAR//RAW/') ('outDir', '/home/pushkalkatara/mr/EdgeML/examples/tf/EMI-RNN/HAR//48_16/') Num train 6339 Num test 2947 Num val 1013 Done

Processing data Extracting features ('subinstanceLen', 48) ('subinstanceStride', 16) ('sourceDir', '/home/pushkalkatara/mr/EdgeML/examples/tf/EMI-RNN/HAR//RAW/') ('outDir', '/home/pushkalkatara/mr/EdgeML/examples/tf/EMI-RNN/HAR//48_16/') x_train 6335 Num train 6335 Num test 2947 Num val 1017 Done

adityakusupati · 2019-08-19T07:36:00Z

Technically this should work. However I had some bad experience using clone.detach where gradient did flow through. Can you please check/debug before you commit. Aditya

…

On Mon, Aug 19, 2019, 9:18 AM Pushkal Katara ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In edgeml_pytorch/trainer/bonsaiTrainer.py <#120 (comment)>: > @@ -126,13 +126,13 @@ def runHardThrsd(self): __thrsdT).to(self.device) self.__thrsdW = torch.FloatTensor( - np.copy(__thrsdW)).to(self.device) Maybe we can use self.__thrsdW = torch.FloatTensor( __thrsdW.clone().detach().to(self.device) I believe it would make a copy of __thrsdW(with data and reference in the computational graph) and further, .detach() would remove the link too and disable differentiation. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#120?email_source=notifications&email_token=AECUG3JRKTFNC5YOSOGQG7DQFIJYNA5CNFSM4IMQ3OLKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCB4HT5Y#discussion_r315030417>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AECUG3IKTWE7JXRVWTZ36MLQFIJYNANCNFSM4IMQ3OLA> .

adityakusupati

@pushkalkatara can we add two more num params. One for the biases and one for scalars.
num_biases, num_scalars

pushkalkatara · 2019-08-19T21:59:22Z

@pushkalkatara can we add two more num params. One for the biases and one for scalars.
num_biases, num_scalars

@adityakusupati Where should I add the num params?

harsha-simhadri · 2019-08-20T04:44:10Z

edgeml_pytorch/graph/rnn.py

@@ -70,16 +70,16 @@ def __init__(self, input_size, hidden_size,
        self._hidden_size = hidden_size
        self._gate_nonlinearity = gate_nonlinearity
        self._update_nonlinearity = update_nonlinearity
-        #self._num_weight_matrices = num_weight_matrices
+        #self._num_weight_matrices = [1,1]


Is it better to set it to None, as opposed to [1,1]? @adityakusupati

@harsha-simhadri that line has been commented out, but it should be None.

We can remove it also, as self._num_weight_matrices is assigned here https://github.com/pushkalkatara/EdgeML/blob/8a1189065b09a8c84abebb8e2250a7c3c2bb571d/edgeml_pytorch/graph/rnn.py#L228

@pushkalkatara should be part of the base RNN class hence we need to assign None here.

Okay, I'll commit the change.

@pushkalkatara I made some suggestion in a new review please do those change and I will do a comprehensive review today.

edgeml_pytorch/graph/rnn.py

adityakusupati · 2019-08-20T07:01:36Z

@pushkalkatara can we add two more num params. One for the biases and one for scalars.
num_biases, num_scalars

@adityakusupati Where should I add the num params?

@pushkalkatara
Ignore these two comments. Please check the review I made #120 (review)

edgeml_pytorch/trainer/bonsaiTrainer.py

pushkalkatara · 2019-08-20T10:29:40Z

@harsha-simhadri @adityakusupati I am not able to test SRNN as the script process_google.py gives out MemoryError while preparing the dataset.

Traceback (most recent call last):
  File "process_google.py", line 257, in <module>
    numFilt, samplerate, winlen, winstep)
  File "process_google.py", line 173, in extractFeatures
    allSamples = np.zeros((len(fileList), maxlen))
MemoryError

Most probably enough memory is not available on my system to generate the maxlen size zeros numpy array.
We can port the TensorFlow version of EMIRNN to PyTorch to test the implementation of rnn.py. Is the example required in PyTorch?

edgeml_pytorch/graph/rnn.py

adityakusupati · 2019-08-20T10:57:52Z

edgeml_pytorch/graph/rnn.py

        if uRank is not None:
            self._num_U_matrices += 1
+            self._num_weight_matrices[1] = self._num_U_matrices
+        if uRank and wRank:
+            self._num_biases += 1


@pushkalkatara num_biases is independent on uRank and wRank but rather dependent on the bias parameters which look like self.bias_*. Please update this accordingly. For FastGRNN it will be 2 bias terms, FastRNN has 1, UGRNN as 2, GRU has 3 and LSTM has 4, simple RNN has 1

Also please check getVars() function to see what we are trying to do with this num_biases. This counting list is a easy way to access getVars()

Yes, I just noticed it. I'll make the changes accordingly.

edgeml_pytorch/graph/rnn.py

adityakusupati · 2019-08-20T10:58:31Z

edgeml_pytorch/graph/rnn.py

@@ -70,16 +70,16 @@ def __init__(self, input_size, hidden_size,
        self._hidden_size = hidden_size
        self._gate_nonlinearity = gate_nonlinearity
        self._update_nonlinearity = update_nonlinearity
-        #self._num_weight_matrices = num_weight_matrices
+        #self._num_weight_matrices = [1,1]


@pushkalkatara I made some suggestion in a new review please do those change and I will do a comprehensive review today.

adityakusupati

Can you please fix the rest of the cells as well. Thanks.

pushkalkatara · 2019-08-20T12:07:02Z

Can you please fix the rest of the cells as well. Thanks.

All others have the correct num_biases in the constructor.

harsha-simhadri · 2019-08-20T12:41:45Z

@harsha-simhadri @adityakusupati I am not able to test SRNN as the script process_google.py gives out MemoryError while preparing the dataset.
Traceback (most recent call last):
  File "process_google.py", line 257, in <module>
    numFilt, samplerate, winlen, winstep)
  File "process_google.py", line 173, in extractFeatures
    allSamples = np.zeros((len(fileList), maxlen))
MemoryError
Most probably enough memory is not available on my system to generate the maxlen size zeros numpy array.
We can port the TensorFlow version of EMIRNN to PyTorch to test the implementation of rnn.py. Is the example required in PyTorch?

Lets hold off on porting EMIRNN to PyTorch just yet. @metastableB Any clue what might be going wrong here?

metastableB · 2019-08-20T13:41:41Z

@harsha-simhadri @pushkalkatara Yes lets hold of on porting EMI-RNN for now. That will require a lot of care.

I'll fix the process_google.py to work out of disk rather than out of RAM today. That should fix the RAM issues. @pushkalkatara thanks for pointing this out.

SachinG007 · 2019-08-20T15:59:26Z

While testing fastcell_example.py

    if cell == "FastGRNN":
        FastCell = FastGRNNCell(inputDims, hiddenDims,
                                gate_non_linearity=gate_non_linearity,
                                update_non_linearity=update_non_linearity,
                                wRank=wRank, uRank=uRank)

unexpected keyword gate_non_linearity
In the function definitions, the key is gate_nonLinearity
(same issue with all other cell Types)
@adityakusupati

pushkalkatara · 2019-08-20T16:13:40Z

@SachinG007 The fix is in the commit 08a3826 in this PR

adityakusupati · 2019-08-20T18:32:57Z

@harsha-simhadri this PR looks good to me in the context of Bonsai and FastCells. Please, approve. @pushkalkatara thanks for your contributions.

harsha-simhadri · 2019-08-21T04:41:33Z

@harsha-simhadri @pushkalkatara Yes lets hold of on porting EMI-RNN for now. That will require a lot of care.

I'll fix the process_google.py to work out of disk rather than out of RAM today. That should fix the RAM issues. @pushkalkatara thanks for pointing this out.

@metastableB Should we wait for your fix or go ahead with PR?

harsha-simhadri · 2019-08-21T09:28:57Z

process_google.py needs to be fixed while working with small memory. But lets do another PR for that. Lets just go ahead now

pushkalkatara · 2019-08-21T13:12:44Z

Thanks for the merge.

pushkalkatara added 7 commits August 18, 2019 00:57

Doc Fix - EMI-RNN.md

506680c

Added relative links

64fe8d0

FastCells Doc Fix

1595a02

IHT Routine Fix microsoft#119

bd49385

FastCell GRNN example fix

08a3826

ProtoNN Example Fix

5371ff0

EMIRNN example fixes

c7a2cb7

This was referenced Aug 18, 2019

IHT routine failing due to numpy call on torch tensor #119

Closed

Testing pytorch implementations. #117

Closed

harsha-simhadri reviewed Aug 18, 2019

View reviewed changes

edgeml_pytorch/trainer/bonsaiTrainer.py Show resolved Hide resolved

harsha-simhadri reviewed Aug 18, 2019

View reviewed changes

adityakusupati suggested changes Aug 19, 2019

View reviewed changes

remove reference from computational graph - .detach()

8a11890

harsha-simhadri reviewed Aug 20, 2019

View reviewed changes

adityakusupati reviewed Aug 20, 2019

View reviewed changes

edgeml_pytorch/graph/rnn.py Outdated Show resolved Hide resolved

edgeml_pytorch/graph/rnn.py Show resolved Hide resolved

edgeml_pytorch/graph/rnn.py Outdated Show resolved Hide resolved

adityakusupati reviewed Aug 20, 2019

View reviewed changes

edgeml_pytorch/trainer/bonsaiTrainer.py Outdated Show resolved Hide resolved

pushkalkatara added 2 commits August 20, 2019 14:10

detach followed by clone

414273d

adding num_biases

819976e

adityakusupati reviewed Aug 20, 2019

View reviewed changes

pushkalkatara added 2 commits August 20, 2019 17:02

Merge branch 'harsha/reorg' into harsha/reorg

dc9bb1b

resolve num_bias

9351a16

adityakusupati reviewed Aug 20, 2019

View reviewed changes

pushkalkatara changed the title ~~Reoraganisation Fixes~~ Reorganisation Fixes Aug 20, 2019

harsha-simhadri merged commit 40663ca into microsoft:harsha/reorg Aug 21, 2019

pushkalkatara deleted the harsha/reorg branch August 21, 2019 13:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reorganisation Fixes #120

Reorganisation Fixes #120

pushkalkatara commented Aug 18, 2019

harsha-simhadri Aug 18, 2019

pushkalkatara Aug 18, 2019

adityakusupati commented Aug 19, 2019 via email

adityakusupati left a comment

pushkalkatara commented Aug 19, 2019

harsha-simhadri Aug 20, 2019

adityakusupati Aug 20, 2019

pushkalkatara Aug 20, 2019

adityakusupati Aug 20, 2019

pushkalkatara Aug 20, 2019

adityakusupati Aug 20, 2019

adityakusupati commented Aug 20, 2019

pushkalkatara commented Aug 20, 2019

adityakusupati Aug 20, 2019

adityakusupati Aug 20, 2019

pushkalkatara Aug 20, 2019

adityakusupati Aug 20, 2019

adityakusupati left a comment

pushkalkatara commented Aug 20, 2019

harsha-simhadri commented Aug 20, 2019

metastableB commented Aug 20, 2019

SachinG007 commented Aug 20, 2019 •

edited

pushkalkatara commented Aug 20, 2019

adityakusupati commented Aug 20, 2019

harsha-simhadri commented Aug 21, 2019

harsha-simhadri commented Aug 21, 2019

pushkalkatara commented Aug 21, 2019

Reorganisation Fixes #120

Reorganisation Fixes #120

Conversation

pushkalkatara commented Aug 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adityakusupati commented Aug 19, 2019 via email

adityakusupati left a comment

Choose a reason for hiding this comment

pushkalkatara commented Aug 19, 2019

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

adityakusupati commented Aug 20, 2019

pushkalkatara commented Aug 20, 2019

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

adityakusupati left a comment

Choose a reason for hiding this comment

pushkalkatara commented Aug 20, 2019

harsha-simhadri commented Aug 20, 2019

metastableB commented Aug 20, 2019

SachinG007 commented Aug 20, 2019 • edited

pushkalkatara commented Aug 20, 2019

adityakusupati commented Aug 20, 2019

harsha-simhadri commented Aug 21, 2019

harsha-simhadri commented Aug 21, 2019

pushkalkatara commented Aug 21, 2019

SachinG007 commented Aug 20, 2019 •

edited