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

Changes in the documentation for synaptic elements and growth curve #219

Merged
merged 7 commits into from
May 23, 2016

Conversation

sdiazpier
Copy link
Contributor

This PR addresses issue #217 by adding documentation for synaptic elements and growth curves in a format suitable for the online documentation generator.

@heplesser
Copy link
Contributor

@sdiazpier Thank you for the new documentation. But I had thought that this documentation should come in addition to, not instead of the doxygen comments describing the code. Could you re-instate those? I am also not sure it makes sense to add user-level (SLI) documentation to the growth_curve base class, since the user never creates an abstract growth curve. Parameter names in the user documentation should be written as the user would use them (no trailing underscores) and should only list the parameters the user can access with SetStatus/GetStatus. The documentation should also give units and default values where appropriate.

@sdiazpier
Copy link
Contributor Author

Dear @heplesser, I have corrected the documentation following your comments. Please let me know if there is anything else that could be improved.

Parameters:
eps double - The target calcium concentration (firing rate) that
the neuron should look to achieve by creating or deleting
synaptic elements.
Copy link
Contributor

Choose a reason for hiding this comment

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

  • I am a little confused here: why is a calcium concentration a firing rate?
  • What is the unit of the Ca concentration?
  • Is eps the same as "epsilon" in the equation?
  • Could you also document parameter nu?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dear @heplesser sorry for the confusion. I have rephrased that sentence so it is clear that the calcium concentration is an indirect measure of the firing rate. The value of nu is actually set in the SynapticElement class but I have extended its documentation in the description section.

@sdiazpier
Copy link
Contributor Author

Dear @heplesser I have done some changes to the documentation, please let me know if it is clearer and if there is anything else I should add.

@heplesser
Copy link
Contributor

@sdiazpier Sorry for the long delay. The documentation is much improved, but I still have some issues. First, could you change the documentation for growth_curve as I suggested above? Then, I find the [Ca2+] in the documentation confusing and think they should be removed almost everywhere. E.g., "eps is the target calcium concentration"---no [Ca2+] needed here.
Concerning the update rule for the calcium concentration, you write "... + beta_Ca if the neuron fires". How exactly is the "if" meant here? If I remember correctly, structural plasticity is only updated a larger time intervals. Does the beta_Ca term apply if the neuron fired a spike since the previous structural plasticity update? What happens if it fired several spikes? Do you add beta_Ca once per spike? Looking at the original paper, Eq 3, I am wondering if that equation is not written incorrectly? If I understand the paper text right, then the calcium concentration jumps by beta_Ca each time the neuron fires a spike. That would imply a delta-function on the right hand side, not just a constant, or? The unit ms^-1 for beta also does not make sense (in the paper).

Finally, I am still wondering about the units of the concentrations and quantities involving concentrations. It seems concentration is unitless, but should it not be given in mol/l or something similar?

Maybe @abigailm could also weigh in on this?

@sdiazpier
Copy link
Contributor Author

Hi dear @heplesser I am sorry for the late response.
Regarding the update rule, the algorithm takes into account things that happen in two different time scales. The calcium concentration is updated on every step of the simulation (increases by beta_Ca when the neuron spikes and decays otherwise), but the number of synaptic elements and the connectivity is only updated on a larger time interval (structural_plasticity_update_interval).
@naveau and I have asked about the units for the Calcium concentration to Dr. Butz and he said that it is unitless in this implementation as it is only used as an indicator of the average electrical activity. We could think more about this issue.
I will do the changes you suggest and remove the [Ca2+] and push my changes ASAP.

@heplesser
Copy link
Contributor

@sdiazpier Travis checks failed due to formatting issues. Could you do the following:

  1. In the top source code directory, run extras/check_code_style.sh
  2. It should tell you, towards the end, that some files violate formatting guidelines and suggest a solution
  3. Run clang-format-3.6 -i FILENAME on each file with formatting issues
  4. Afterwards, take a look especially at SLI documentation and fix indentation of lines. It should look like with respect to alignement
V_m  double - The membrane potential
              which is given in mV.
  1. Commit your changes.

@sdiazpier
Copy link
Contributor Author

Dear @heplesser I have finally corrected the issues with the formatting. Please let me know if you think any further changes would be required.

All the best,
Sandra

@heplesser
Copy link
Contributor

@sdiazpier Thank you for fixing this! 👍 from me now.
@abigailm, if you are also happy, this is ready to be merged.

@abigailm
Copy link
Contributor

👍 and merging

@abigailm abigailm merged commit 5b80b8d into nest:master May 23, 2016
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

Successfully merging this pull request may close these issues.

3 participants