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

Fix init permanence distance from connected threshold: TM, SP #889

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

Conversation

breznak
Copy link
Member

@breznak breznak commented Sep 14, 2020

TM initial and connected synaptic weights were mistakenly (by our changes in the past) initialized too far away. The intention was/is to init close to the edge of "connected".

  • fixed in TM
  • added init check to avoid this.
  • SP should initialize closer to the middle (now 0.2 from 0.1, ideally move to 0.5 all)

This is a minor PR, ready to go as is. Changes doublechecked on NAB, no improvement/worse.

PS: As a follow up, I'd like to simplify the synapse init logic:

  • drop Connection's argument connectedPermanence and hardcode this value (empirically no difference in 0.2, 0.5)
  • in SP, TM drop related args for connected threshold and initial synapse.

"close" measured in the number of needed permanence inc/dec steps.
Not initializing close leads to suboptimal functionality.
@breznak breznak added the ready label Sep 14, 2020
@breznak breznak requested review from dkeeney and Zbysekz September 14, 2020 16:26
@breznak breznak self-assigned this Sep 14, 2020
@breznak
Copy link
Member Author

breznak commented Sep 14, 2020

There's a number of tests that are expected to fail and I'll have to update them. But the essence of the PR is here.

Copy link

@dkeeney dkeeney left a comment

Choose a reason for hiding this comment

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

I see no obvious problems. I cannot judge on the algorithm.

Copy link

@Zbysekz Zbysekz left a comment

Choose a reason for hiding this comment

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

I also see no problem.

@Zbysekz
Copy link

Zbysekz commented Sep 15, 2020

PS: As a follow up, I'd like to simplify the synapse init logic:

drop Connection's argument connectedPermanence and hardcode this value (empirically no difference in 0.2, 0.5)
in SP, TM drop related args for connected threshold and initial synapse.

This would be nice, since it is simplification.

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

Successfully merging this pull request may close these issues.

3 participants