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

Truncate semantic pointer names to 1 KB #246

Merged
merged 3 commits into from
Jun 14, 2020
Merged

Truncate semantic pointer names to 1 KB #246

merged 3 commits into from
Jun 14, 2020

Conversation

arvoelke
Copy link
Contributor

@arvoelke arvoelke commented May 1, 2020

Motivation and context:
Fixes #244. 1 KB was chosen somewhat arbitrarily, but it seems long enough to capture the majority of use cases where someone might want to read the name, while being a cheap amount of memory relative to the resource requirements of the semantic pointer itself and how it will be used.

I considered adding a warning, but it didn't seem important enough to let the user know (the fact that the name is being truncated seems inconsequential for most users)?

How has this been tested?
Added a unit test similar to the reproducing example from #244.

How long should this take to review?

  • Quick (less than 40 lines changed or changes are straightforward)

Types of changes:

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • I have read the CONTRIBUTING.rst document.
  • I have updated the documentation accordingly.
  • I have included a changelog entry.
  • I have added tests to cover my changes.
  • I have run the test suite locally and all tests passed.

Copy link
Collaborator

@jgosmann jgosmann left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM.

Static checks are failing though because the commit message "body is too short". I think it is fine, but probably something that needs to be fixed on the nengo-bones side?

@hunse
Copy link
Collaborator

hunse commented May 26, 2020

We may make that change in nengo-bones at some point. For now, though, I would recommend just changing the message to e.g. "This commit fixes #???" so that it's long enough.

@tbekolay
Copy link
Member

We did decide to remove the commit body minimum length, but it'll take some time before that change is done as it's a ways down our priority list.

What I would do is copy in the context that is in the first post of this PR into the commit message. What's the harm in having additional context in the commit message body?

@arvoelke
Copy link
Contributor Author

What I would do is copy in the context that is in the first post of this PR into the commit message.

Done.

@arvoelke
Copy link
Contributor Author

3: B1 Line exceeds max length (285>72): "Fixes #244. 1 KB was chosen somewhat arbitrarily ...

Trying again.

@arvoelke
Copy link
Contributor Author

/home/travis/build/nengo/nengo-spa/docs/license.rst.rst:31:broken link: http://www.numpy.org/license.html (404 Client Error: Not Found for url: https://numpy.org/license.html)

Trying again..

@arvoelke
Copy link
Contributor Author

$ codespell -q 3 --skip=./build,*/_build,*-checkpoint.ipynb,./.eggs,./.git,*/_vendor --ignore-words-list=ba
./docs/user-guide/spa-intro.rst:129: contex ==> context
COMMAND 'codespell' FAILED```

Trying again...

@arvoelke arvoelke removed the blocked label May 29, 2020
@arvoelke
Copy link
Contributor Author

It passed (unblocked)!

@jgosmann jgosmann added this to the 1.0.2 milestone Jun 10, 2020
@arvoelke
Copy link
Contributor Author

Note: will need to drop the last two commits after #252.

arvoelke added 3 commits June 14, 2020 20:29
Fixes #244. 1 KB seems long enough to capture the majority of use cases
where someone might want to read the name, while being a cheap amount
of memory relative to the resource requirements of the semantic pointer
itself and how it will be used.
This was referenced Jun 14, 2020
@jgosmann jgosmann merged commit 810fbfc into master Jun 14, 2020
@jgosmann jgosmann deleted the fix-244 branch June 14, 2020 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

Semantic pointer names may consume all RAM when iterating/recursing
4 participants