Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change copy functions over to use (de)serialization feature #98

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

enginefeeder101
Copy link
Contributor

The (de)serialzation functions already have all the code needed to make a deep copy
of the NeuralNetwork. This commit also adds a test for the Matrix.copy() function.
This also copies the learning rate and if #97 is merged the activation function is
copied as well.

The (de)serialzation functions already have all the code needed to make a deep copy
of the NeuralNetwork. This commit also adds a test for the `Matrix.copy()` function.
This also copies the learning rate and if CodingTrain#97 is merged the activation function is
copied as well.
Copy link
Member

@shiffman shiffman left a comment

Choose a reason for hiding this comment

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

Ah, of course, thank you for this! While I agree this has its advantages, I think I might like to hold off on merging until I make a video discussing serialize and deserialize? I'm conflicted on how to handle these improvements which are excellent but then result in the code not matching the video tutorials. Maybe after I get through my examples I can merge a lot of these and make a single video going over the improvements.

expect(n).toEqual({
rows: m.rows,
cols: m.cols,
data: m.data
Copy link
Member

Choose a reason for hiding this comment

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

will this work? Won't we have to check the values of the arrays themselves as opposed to the data reference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does, as this does not pass:

  let n = m.copy();
  m.add(1);

  expect(n).toEqual({
    rows: m.rows,
    cols: m.cols,
    data: m.data
  });

schermafdruk_2018-03-10_18-22-24

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually expect(n).toEqual(m); works as well 😆

Copy link
Member

Choose a reason for hiding this comment

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

Oh cool! So Jest does something like a "deep" comparison behind the scenes?

@shiffman
Copy link
Member

If you want to submit the unit tests separately I can merge that now.

@enginefeeder101
Copy link
Contributor Author

enginefeeder101 commented Mar 10, 2018

I pushed PR #99 including just the test so you can merge those.

Copy tests are implemented in CodingTrain#99. Therefore removed from this branch
to be able to merge this neatly.
@enginefeeder101
Copy link
Contributor Author

I removed the added tests (see #99) from this branch to allow you to smoothly merge his PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants