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

Erfc neuron model #492

Merged
merged 19 commits into from Mar 23, 2018

Conversation

Projects
None yet
7 participants
@tobikausk
Contributor

tobikausk commented Sep 23, 2016

Add the neuron model "erfc-neuron" including the necessary tests. "erfc-neuron" is derived from the binary-neuron template and has got a complentary error function as activation function.
I propose @mhelias and @heplesser as reviewers.

@tobikausk tobikausk changed the title from Erfc neuron model to [WIP]Erfc neuron model Sep 23, 2016

@tobikausk tobikausk changed the title from [WIP]Erfc neuron model to Erfc neuron model Sep 27, 2016

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Sep 29, 2016

Contributor

@tobikausk Please fix the C++ and Python formatting errors.

Contributor

heplesser commented Sep 29, 2016

@tobikausk Please fix the C++ and Python formatting errors.

@jakobj

This comment has been minimized.

Show comment
Hide comment
@jakobj

jakobj Oct 3, 2016

Contributor

seems like there are still formatting errors:

PEP8 analysis: (only first occurrence)
   ---------------------------------------
   | pynest/nest/tests/test_erfc_neuron.py:
   |   pynest/nest/tests/test_erfc_neuron.py:107:21: E128 continuation line under-indented for visual indent
   |   pynest/nest/tests/test_erfc_neuron.py:108:21: E122 continuation line missing indentation or outdented

it's possible that you have to fix them by hand if auto-pep8 does not catch them.

Contributor

jakobj commented Oct 3, 2016

seems like there are still formatting errors:

PEP8 analysis: (only first occurrence)
   ---------------------------------------
   | pynest/nest/tests/test_erfc_neuron.py:
   |   pynest/nest/tests/test_erfc_neuron.py:107:21: E128 continuation line under-indented for visual indent
   |   pynest/nest/tests/test_erfc_neuron.py:108:21: E122 continuation line missing indentation or outdented

it's possible that you have to fix them by hand if auto-pep8 does not catch them.

@terhorstd terhorstd requested review from heplesser and DimitriPlotnikov Jan 23, 2017

@DimitriPlotnikov

This comment has been minimized.

Show comment
Hide comment
@DimitriPlotnikov

DimitriPlotnikov Jun 19, 2017

Hello,

could you, please, resolve conflicts?

DimitriPlotnikov commented Jun 19, 2017

Hello,

could you, please, resolve conflicts?

@tobikausk

This comment has been minimized.

Show comment
Hide comment
@tobikausk

tobikausk Jun 23, 2017

Contributor

We fixed all conflicts. Please have another look on what we did.

Contributor

tobikausk commented Jun 23, 2017

We fixed all conflicts. Please have another look on what we did.

@heplesser

@tobikausk This looks pretty fine, just a number of details, mainly in comments/documentation.

Show outdated Hide outdated nestkernel/nest_names.h
Show outdated Hide outdated pynest/nest/tests/test_erfc_neuron.py
Show outdated Hide outdated pynest/nest/tests/test_erfc_neuron.py
Show outdated Hide outdated models/erfc_neuron.h
Show outdated Hide outdated models/erfc_neuron.h
Show outdated Hide outdated models/erfc_neuron.h
Show outdated Hide outdated models/erfc_neuron.h
Show outdated Hide outdated models/erfc_neuron.h
Show outdated Hide outdated models/erfc_neuron.h
Show outdated Hide outdated models/erfc_neuron.h
@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Aug 9, 2017

Contributor

@tobikausk There are still a number of clang-format errors and the problem with the _impl.h file being included in a header file. If the latter should be necessary, please explain why.

Contributor

heplesser commented Aug 9, 2017

@tobikausk There are still a number of clang-format errors and the problem with the _impl.h file being included in a header file. If the latter should be necessary, please explain why.

@tobikausk

This comment has been minimized.

Show comment
Hide comment
@tobikausk

tobikausk Aug 21, 2017

Contributor

@heplesser @jakobj
Thanks a lot for the very detailed commentary!
Concerning the question of the _impl.h-files, however, Jakob and I concluded that they have to be included in the erfc_neuron.h-file as it includes function definitions of the binary neuron template, which the linker otherwise misses. Therefore, we would keep this line, analogous to the case of the Ginzburg and the McCulloch-Pitts neuron.

Contributor

tobikausk commented Aug 21, 2017

@heplesser @jakobj
Thanks a lot for the very detailed commentary!
Concerning the question of the _impl.h-files, however, Jakob and I concluded that they have to be included in the erfc_neuron.h-file as it includes function definitions of the binary neuron template, which the linker otherwise misses. Therefore, we would keep this line, analogous to the case of the Ginzburg and the McCulloch-Pitts neuron.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Aug 23, 2017

Contributor

@tobikausk "Umgekehrt wird ein Schuh daraus", as the German proverb says: Since binary_neuron_impl.h is always included together with binary_neuron.h, because the impl file provides essential definitions of templated methods, there should be no binary_neuron_impl.h file: all the code in that file should be included in binary_neuron.h. In this way we avoid the rather confusing impl file.

Could you also merge the most recent changes from master and resolve the conflicts mentioned above?

Contributor

heplesser commented Aug 23, 2017

@tobikausk "Umgekehrt wird ein Schuh daraus", as the German proverb says: Since binary_neuron_impl.h is always included together with binary_neuron.h, because the impl file provides essential definitions of templated methods, there should be no binary_neuron_impl.h file: all the code in that file should be included in binary_neuron.h. In this way we avoid the rather confusing impl file.

Could you also merge the most recent changes from master and resolve the conflicts mentioned above?

@jakobj

This comment has been minimized.

Show comment
Hide comment
@jakobj

jakobj Aug 23, 2017

Contributor

@heplesser: we have reached exactly the same conclusion (that there should be no _impl.h file), however changing this would go well beyond the current PR, hence a reasonable strategy is to provide the new model in the same style as the old models and create a separate PR addressing the issue of the _impl.h.
The (questionable) reasoning behind the structure of separate .h and _impl.h files seems to be in this case the separation of inline functions and regular functions, tracing back before the move to git:

$git blame models/binary_neuron_impl.h | head -n1
^c229212d (Tiziano Zito    2015-05-11 17:11:02 +0200   1) /*

I agree that this should be fixed soon.

Contributor

jakobj commented Aug 23, 2017

@heplesser: we have reached exactly the same conclusion (that there should be no _impl.h file), however changing this would go well beyond the current PR, hence a reasonable strategy is to provide the new model in the same style as the old models and create a separate PR addressing the issue of the _impl.h.
The (questionable) reasoning behind the structure of separate .h and _impl.h files seems to be in this case the separation of inline functions and regular functions, tracing back before the move to git:

$git blame models/binary_neuron_impl.h | head -n1
^c229212d (Tiziano Zito    2015-05-11 17:11:02 +0200   1) /*

I agree that this should be fixed soon.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Aug 24, 2017

Contributor

@jakobj @tobikausk I agree with you that removing the impl file is a separate matter and should not bloat this PR. At the same time, I think it would be good to fix the impl-issue first, instead of adding new code with the impl.

Eliminating the impl file is just a matter of copying the code in the impl file to the header file and removing existing #include "binary_neuron_impl.h files, plus updating models/CMakeLists.txt. Could you create a PR for that?

I would preferably merge that first, then update this PR correspondingly, so we avoid introducing new code which we know needs changing.

Contributor

heplesser commented Aug 24, 2017

@jakobj @tobikausk I agree with you that removing the impl file is a separate matter and should not bloat this PR. At the same time, I think it would be good to fix the impl-issue first, instead of adding new code with the impl.

Eliminating the impl file is just a matter of copying the code in the impl file to the header file and removing existing #include "binary_neuron_impl.h files, plus updating models/CMakeLists.txt. Could you create a PR for that?

I would preferably merge that first, then update this PR correspondingly, so we avoid introducing new code which we know needs changing.

@jakobj jakobj referenced this pull request Aug 24, 2017

Merged

Remove binary_neuron_impl.h #813

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Aug 24, 2017

Contributor

Thanks for #813! Merging this PR should wait until #813 is merged into this PR.

Contributor

heplesser commented Aug 24, 2017

Thanks for #813! Merging this PR should wait until #813 is merged into this PR.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Aug 24, 2017

Contributor

#813 is merged to master, so updating this PR with all the newest changes from Master should get this PR ready for merging, too.

Contributor

heplesser commented Aug 24, 2017

#813 is merged to master, so updating this PR with all the newest changes from Master should get this PR ready for merging, too.

@heplesser heplesser added P: PR Created and removed P: Blocked labels Aug 24, 2017

@tobikausk

This comment has been minimized.

Show comment
Hide comment
@tobikausk

tobikausk Sep 20, 2017

Contributor

@jakobj @heplesser @DimitriPlotnikov
Thank you very much for your help and for pushing me forward... I hope that the result is fine now.

Contributor

tobikausk commented Sep 20, 2017

@jakobj @heplesser @DimitriPlotnikov
Thank you very much for your help and for pushing me forward... I hope that the result is fine now.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Oct 30, 2017

Contributor

@tobikausk Could you fix the code formatting problems that make Travis fail?

Contributor

heplesser commented Oct 30, 2017

@tobikausk Could you fix the code formatting problems that make Travis fail?

@tobikausk

This comment has been minimized.

Show comment
Hide comment
@tobikausk

tobikausk Dec 20, 2017

Contributor

@heplesser Fixed formatting issues, together with @jakobj .

Contributor

tobikausk commented Dec 20, 2017

@heplesser Fixed formatting issues, together with @jakobj .

@heplesser heplesser requested review from hakonsbm and removed request for DimitriPlotnikov Dec 21, 2017

@hakonsbm

This looks pretty good to me, just a couple typos that should be fixed.

Show outdated Hide outdated models/erfc_neuron.h
Show outdated Hide outdated models/erfc_neuron.h
@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Jan 12, 2018

Contributor

@tobikausk: could you please fix the typos pointed out by @hakonsbm, so we can merge this? Thanks!

Contributor

jougs commented Jan 12, 2018

@tobikausk: could you please fix the typos pointed out by @hakonsbm, so we can merge this? Thanks!

tobikausk and others added some commits Mar 8, 2018

@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Mar 9, 2018

Contributor

I really wonder if we shouldn't just reformat the whole nest_names.h file in the format

/** Description for variable */
extern const Name variable;

Using the current block format it seems to be impossible to arrive at a stable format of that file that

  1. clang-format is not reformatting again and again on each call and
  2. is still below (any reasonable) line-length limit

IMO this would best be done in another PR and merged to this one.

Contributor

jougs commented Mar 9, 2018

I really wonder if we shouldn't just reformat the whole nest_names.h file in the format

/** Description for variable */
extern const Name variable;

Using the current block format it seems to be impossible to arrive at a stable format of that file that

  1. clang-format is not reformatting again and again on each call and
  2. is still below (any reasonable) line-length limit

IMO this would best be done in another PR and merged to this one.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Mar 10, 2018

Contributor

@jougs I very much like your suggestion! Could you implement it quickly? I guess it could be scripted ...

Contributor

heplesser commented Mar 10, 2018

@jougs I very much like your suggestion! Could you implement it quickly? I guess it could be scripted ...

@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Mar 12, 2018

Contributor

@heplesser, please see #909 for a PR that fixes the problem with Names and their comments. As most of the comments were actually wrong or outdated, I decided to remove them altogether instead of reformatting. I think that this is better, as many names are used in different places and their meaning should thus rather be defined and documented in the model.

Contributor

jougs commented Mar 12, 2018

@heplesser, please see #909 for a PR that fixes the problem with Names and their comments. As most of the comments were actually wrong or outdated, I decided to remove them altogether instead of reformatting. I think that this is better, as many names are used in different places and their meaning should thus rather be defined and documented in the model.

@heplesser heplesser added this to the NEST 2.16 milestone Mar 19, 2018

Merge branch 'master' into tobikausk_erfc_neuron_model
# Conflicts:
#	nestkernel/nest_names.h
@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Mar 21, 2018

Contributor

@tobikausk: can you please merge master? That should solve the failing checks and we could merge this.

Contributor

jougs commented Mar 21, 2018

@tobikausk: can you please merge master? That should solve the failing checks and we could merge this.

Merge pull request #1 from heplesser/tobikausk_erfc_neuron_model
Fix merge and formatting issues for erfc neuron

@heplesser heplesser merged commit 6eac560 into nest:master Mar 23, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment