Documentation improvement to Hill-Tononi neuron and minor code prettification #387

Merged
merged 11 commits into from Aug 8, 2016

Conversation

Projects
None yet
5 participants
@heplesser
Contributor

heplesser commented Jun 7, 2016

  • Improved documentation of mathematical details in HT neuron.
  • Reorderd some statements, removed unnecessary namespace qualifiers, added const where possible.
  • Renamed some variables.
  • Switched orded of operands in prefactor and peak_value so all expressions are positive.
@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Jun 20, 2016

Contributor

@ingablundell Could you review this one?

Contributor

heplesser commented Jun 20, 2016

@ingablundell Could you review this one?

S_.y_[ 2 + 2 * i ] +=
V_.cond_steps_[ i ] * B_.spike_inputs_[ i ].get_value( lag );
+ }

This comment has been minimized.

@DimitriPlotnikov

DimitriPlotnikov Jul 27, 2016

It is really hard to understand the intention of this loop (an update of DG_AMPA, DG_NMDA, DG_GABA_A, DG_GABA_B state variables). Did you think about unrolling of the loop and the usage of explicit names of state variables would improve comprehensibility of the code?

@DimitriPlotnikov

DimitriPlotnikov Jul 27, 2016

It is really hard to understand the intention of this loop (an update of DG_AMPA, DG_NMDA, DG_GABA_A, DG_GABA_B state variables). Did you think about unrolling of the loop and the usage of explicit names of state variables would improve comprehensibility of the code?

This comment has been minimized.

@DimitriPlotnikov

DimitriPlotnikov Jul 27, 2016

Another possible improvement:
In the original model there is a TODO (line 577, ht_neuron.cpp):

// NOTE: code below initializes conductance step size for incoming pulses.
// Variable and function names need to be changed!

Since you are already working on the ht_model, could your improve this?

@DimitriPlotnikov

DimitriPlotnikov Jul 27, 2016

Another possible improvement:
In the original model there is a TODO (line 577, ht_neuron.cpp):

// NOTE: code below initializes conductance step size for incoming pulses.
// Variable and function names need to be changed!

Since you are already working on the ht_model, could your improve this?

This comment has been minimized.

@heplesser

heplesser Aug 2, 2016

Contributor

@DimitriPlotnikov I see your point, but I like the loop because it is more flexible in case more synapse models would be added. I have added a more detailed comment to clarify things. I also removed the second line of the comment on line 577, since, after looking again at that code, I didn't really think it would make much sense to change function/variable names there.

@heplesser

heplesser Aug 2, 2016

Contributor

@DimitriPlotnikov I see your point, but I like the loop because it is more flexible in case more synapse models would be added. I have added a more detailed comment to clarify things. I also removed the second line of the comment on line 577, since, after looking again at that code, I didn't really think it would make much sense to change function/variable names there.

@ingablundell

This comment has been minimized.

Show comment
Hide comment
@ingablundell

ingablundell Jul 27, 2016

@heplesser The new names of the variabless in ht_neuron.cpp look very sensible to me! Also the ordering of operands seems to be correct. The documentation for the calculation of g_peak/t_peak looks nice. Why does $a$ have to be larger than $b$?

Can you also propose a reviewer for the continuous integration stuff who can look at the build.sh and the parse_travis_log.py files?

@heplesser The new names of the variabless in ht_neuron.cpp look very sensible to me! Also the ordering of operands seems to be correct. The documentation for the calculation of g_peak/t_peak looks nice. Why does $a$ have to be larger than $b$?

Can you also propose a reviewer for the continuous integration stuff who can look at the build.sh and the parse_travis_log.py files?

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Aug 2, 2016

Contributor

@ingablundell Thanks! Strictly speaking, one only needs a != b, and I have changed the comment accordingly. In biological reality, the rise time constant always seems to be shorter than the decay time constant.

I merged all recent changes from master now, so the changes in build.sh and parse_travis_log.py are no longer part of this PR.

Contributor

heplesser commented Aug 2, 2016

@ingablundell Thanks! Strictly speaking, one only needs a != b, and I have changed the comment accordingly. In biological reality, the rise time constant always seems to be shorter than the decay time constant.

I merged all recent changes from master now, so the changes in build.sh and parse_travis_log.py are no longer part of this PR.

@apeyser apeyser merged commit 133bb64 into nest:master Aug 8, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@heplesser heplesser deleted the heplesser:fix_ht_comment branch Aug 26, 2016

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