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.

Sign up for GitHub

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

Jump to bottom

Added protection against probabilistic wiring to spike_detector (fixes #351) #560

Merged
merged 5 commits into from
Dec 19, 2016
Merged

Added protection against probabilistic wiring to spike_detector (fixes #351) #560

merged 5 commits into from
Dec 19, 2016

Conversation

heplesser
Copy link
Contributor

@heplesser heplesser commented Nov 22, 2016 •
edited
Loading

This adresses #351 .

I suggest @hannahbos and @jougs as reviewers.

@mention-bot
Copy link

@heplesser, thanks for your PR! By analyzing the history of the files in this pull request, we identified @otizonaizit, @sdiazpier and @tammoippen to be potential reviewers.

@heplesser heplesser added this to the NEST 2.12 milestone Nov 22, 2016
@heplesser heplesser added ZC: Kernel DO NOT USE THIS LABEL I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) ZP: PR Created DO NOT USE THIS LABEL S: High Should be handled next T: Bug Wrong statements in the code or documentation labels Nov 22, 2016
@heplesser heplesser changed the title Added protection against probabilistic wiring to spike_detector Added protection against probabilistic wiring to spike_detector (fixes #531) Nov 22, 2016
@heplesser heplesser changed the title Added protection against probabilistic wiring to spike_detector (fixes #531) Added protection against probabilistic wiring to spike_detector (fixes #351) Nov 22, 2016
mschmidt87
mschmidt87 reviewed Dec 13, 2016
View reviewed changes
Copy link

@mschmidt87 mschmidt87 left a comment

Choose a reason for hiding this comment

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

I don't have anything to complain. I just wonder if the commit touching .gitignore is supposed to be in this pull-request.

.gitignore
@@ -56,3 +56,4 @@ doc/normaldoc.conf
lib/sli/rcsinfo.sli
extras/emacs/sli.el
extras/nest_vars.sh
*.pyc
Copy link

Choose a reason for hiding this comment

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

Why is this part of the pull request

@heplesser
Copy link
Contributor Author

@mschmidt87 .pyc files should certainly be ignored by Git. I, as probably many other users, had *.pyc in my global gitignore file. After that file caused problems in a different context (not related to pyc at all), I decided not to have a global gitignore file any more. As a consequence, I needed to add *.pyc to project .gitignore files, and this branch was simply the first I committed to after I deleted by global gitignore.

@mschmidt87
Copy link

Maybe I am missing something here, but isn't *.pyc already included in .gitignore, precisely in line 12? So, adding it another time shouldn't be necessary, should it?

@heplesser
Copy link
Contributor Author

@mschmidt87 I guess my 351-branch didn't contain all merges from master and thus didn't have the *.pyc on line 12. I have now removed the duplicate *.pyc.

@mschmidt87
Copy link

mschmidt87 commented Dec 14, 2016 •
edited
Loading

I am not sure how picky we are in general with these things, but it would be cleaner if you would throw away the commits touching .gitignore e.g. by cherry-picking just the first commit of this PR or rebasing. This way, .gitignore would remain completely untouched (currently, you're adding an empty line to the file) and we wouldn't have the unnecessary commits in the history.

@hannahbos
Copy link

hannahbos commented Dec 19, 2016 •
edited
Loading

Great. Thanks! 👍

@heplesser
Copy link
Contributor Author

@mschmidt87 You are right that my changes to .gitignore will leave a bit of unnecessary back-and-forth in NEST history, but at present I would prioritize completion of milestone 2.12 and robustness before history neatness. I am also afraid that attempts at cherry-picking/rebasing might introduce even more of mess, so I hope you are willing to approve this PR as it is?

@mschmidt87
Copy link

mschmidt87 commented Dec 19, 2016 •
edited
Loading

Okay, sure. 👍
I guess, rebasing would be complicated here, because there are some merge commits from master.

@heplesser heplesser merged commit 9103618 into nest:master Dec 19, 2016
@heplesser heplesser deleted the fix351-random-conn-to-devices branch December 20, 2016 07:18
@heplesser heplesser mentioned this pull request Feb 21, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hannahbos hannahbos hannahbos approved these changes

@mschmidt87 mschmidt87 mschmidt87 approved these changes

Assignees
No one assigned
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: High Should be handled next T: Bug Wrong statements in the code or documentation ZC: Kernel DO NOT USE THIS LABEL ZP: PR Created DO NOT USE THIS LABEL
Projects
None yet
Milestone
NEST 2.12
Development

Successfully merging this pull request may close these issues.
4 participants
@heplesser @mention-bot @mschmidt87 @hannahbos