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

New method Connections::destroyMinPermanenceSynapses #446

Merged
merged 4 commits into from
May 12, 2019

Conversation

ctrl-z-9000-times
Copy link
Collaborator

@ctrl-z-9000-times ctrl-z-9000-times commented May 5, 2019

As discussed in PR #285.

Previous this was a private function of the Temporal Memory.
This is useful and general purpose.
It is now a public method of Connections.

TODO:

  • Documentation
  • Python bindings

Previous this was a private function of the Temporal Memory.
This is useful and general purpose.
It is now a public method of Connections.
@ctrl-z-9000-times ctrl-z-9000-times self-assigned this May 5, 2019
Copy link
Member

@breznak breznak left a comment

Choose a reason for hiding this comment

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

This looks good overall 👍

A few nits for consideration, and 1-2 fixes please

src/nupic/algorithms/Connections.cpp Outdated Show resolved Hide resolved
src/nupic/algorithms/Connections.cpp Show resolved Hide resolved
src/nupic/algorithms/Connections.cpp Outdated Show resolved Hide resolved
src/nupic/algorithms/Connections.cpp Outdated Show resolved Hide resolved
src/nupic/algorithms/TemporalMemory.cpp Show resolved Hide resolved
@ctrl-z-9000-times ctrl-z-9000-times marked this pull request as ready for review May 11, 2019 18:57
@ctrl-z-9000-times ctrl-z-9000-times added ready TM code code enhancement, optimization, cleanup..programmer stuff labels May 11, 2019
Copy link
Member

@breznak breznak left a comment

Choose a reason for hiding this comment

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

This looks good, I like the static TM's method is now useful for anybody in Connections.
Please consider making the unit test for it

const Permanence A_perm = dataForSynapse(A).permanence;
const Permanence B_perm = dataForSynapse(B).permanence;
if( A_perm == B_perm ) {
return A < B;
Copy link
Member

Choose a reason for hiding this comment

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

I see you're already using the determinitstic sort logic from #465 👍

* @param nDestroy - Must be greater than or equal to zero!
* @param excludeCells - Presynaptic cells which will NOT have any synapses destroyed.
*/
void destroyMinPermanenceSynapses(const Segment segment, Int nDestroy,
Copy link
Member

Choose a reason for hiding this comment

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

this method should now have its unit-test, given its now public in Connections.

Copy link
Member

Choose a reason for hiding this comment

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

..probably the test could be copied from TM tests, if there were one

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The TM has unit tests which call this method, albeit indirectly via a method in the TM. I think that the testing coverage is good enough so I'm going to go ahead and merge this.

Thanks for reviewing this!

@ctrl-z-9000-times ctrl-z-9000-times merged commit ae622e6 into master May 12, 2019
@ctrl-z-9000-times ctrl-z-9000-times deleted the destroyMinPermanenceSynapses branch May 12, 2019 14:52
@breznak breznak mentioned this pull request May 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code code enhancement, optimization, cleanup..programmer stuff ready TM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants