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

Code Duplication in SNN::doCurrentUpdate() #12

Closed
SigmaX opened this issue Nov 3, 2020 · 3 comments
Closed

Code Duplication in SNN::doCurrentUpdate() #12

SigmaX opened this issue Nov 3, 2020 · 3 comments

Comments

@SigmaX
Copy link
Contributor

SigmaX commented Nov 3, 2020

While investigating issue #11, I noticed that SNN::doCurrentUpdate() duplicates two near-identical for loops (see here).

This is unusual, and it's not clear whether it is intentional, or was done for a reason. Should be investigated to either correct or add a clarifying comment.

@nmsutton
Copy link

@SigmaX I think that while the loops are very similar the key difference is that doCurrentUpdateD1_GPU(netId) or doCurrentUpdateD2_GPU(netId) are run seperately in them. "D1" is for "updating and generating spikes on connections with a delay of 1ms" and "D2" is for "delays greater than 1". Certainly, the code redundancy in the loops could be improved but I think the loops do two different things. I think perhaps "D2" is purposefully run before "D1". This issue however could stay open if wanted because a non-duplicated code version of the programming could be done to improve the code.

@SigmaX
Copy link
Contributor Author

SigmaX commented Dec 17, 2022

Thanks @nmsutton. I added comments to the equivalent location in CARLsim6 to clarify.

Closing this, as the main point was to make sure there was not a bug.

I would take a stab at removing the redundancy/doing some local cleanup there (like adding function docs, which are needed throughout the file), but I can't tell if that code is covered by unit tests or not. I don't dare refactor things without a low-level test harness.

@SigmaX SigmaX closed this as completed Dec 17, 2022
@nmsutton
Copy link

@SigmaX You're welcome and thanks for adding the code comments.

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

No branches or pull requests

2 participants