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

Update model documentation for iaf_psc_delta model #3086

Merged
merged 12 commits into from
Mar 4, 2024

Conversation

ackurth
Copy link
Contributor

@ackurth ackurth commented Feb 1, 2024

No description provided.

Co-authored-by: jasperalbers <jasperalbers@posteo.de>
@jessica-mitchell jessica-mitchell added T: Enhancement New functionality, model or documentation S: Normal Handle this with default priority I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Feb 1, 2024
@jessica-mitchell jessica-mitchell added this to In progress in Documentation via automation Feb 1, 2024
@jessica-mitchell jessica-mitchell added this to To do in Models via automation Feb 1, 2024
@jessica-mitchell jessica-mitchell changed the title Add model documentation for iaf_psc_delta model Update model documentation for iaf_psc_delta model Feb 1, 2024
Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

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

This looks quite good, see small comments inline.

But it made me also wonder about the units of i_syn, also in the other iaf_psc models. We need to take a look at that.

models/iaf_psc_delta.h Outdated Show resolved Hide resolved
models/iaf_psc_delta.h Outdated Show resolved Hide resolved
models/iaf_psc_delta.h Outdated Show resolved Hide resolved
Documentation automation moved this from In progress to Review Feb 6, 2024
Models automation moved this from To do to Review Feb 6, 2024
@heplesser
Copy link
Contributor

heplesser commented Feb 7, 2024

This looks quite good, see small comments inline.

But it made me also wonder about the units of i_syn, also in the other iaf_psc models. We need to take a look at that.

@ackurth I looked more closely at this (also for the alpha case) and the situation is as follows:

  • We need to provide information on the unit of $w_j$
  • For the delta synapse, is the unit of $w_j$ actually pA or is it rather mV? I'm a bit confused here. This is, I guess related to the question if the membrane potential upon a delta spike input should jump by $w_j$ (which is does at present) or by $w_j/C_m$
  • For all other psc synapses, the connection weight $w_j$ is always interpreted in pA, which means that $i_{syn}$ is dimensionless. Then, in the equations for "The individual post-synaptic currents (PSCs) are given by" and "the total charge transferred", we should always look at $w_j \times i_{syn}$ to that units work out.

What do you think?

ackurth and others added 4 commits February 14, 2024 09:59
Co-authored-by: Hans Ekkehard Plesser <hans.ekkehard.plesser@nmbu.no>
Co-authored-by: Hans Ekkehard Plesser <hans.ekkehard.plesser@nmbu.no>
@ackurth
Copy link
Contributor Author

ackurth commented Feb 15, 2024

I reformulated now trying to avoid the unit issue but to be very explicit regarding what is going on.
Please have a look.
If you don't like it this way I will alter it according to what you requested above.

@heplesser
Copy link
Contributor

I reformulated now trying to avoid the unit issue but to be very explicit regarding what is going on. Please have a look. If you don't like it this way I will alter it according to what you requested above.

This looks much better now. But I find the $\Delta \dot{V}{syn}$ a bit difficult to read. How about just $\dot{Delta}{syn}$?

Concerning $i_{syn}$ and $q$, they now have the units A/V and F, respectively. I think it would make more sense to include $w$ to the equations so we end up with A and F (i.e., pA and pF).

It would also be good to extend "where is the corresponding synaptic weight." to "where is the corresponding synaptic weight in mV."

@ackurth
Copy link
Contributor Author

ackurth commented Feb 16, 2024

This looks much better now. But I find the $\Delta \dot{V}{syn}$ a bit difficult to read. How about just $\dot{Delta}{syn}$?
Done

Concerning isyn and q, they now have the units A/V and F, respectively. I think it would make more sense to include w to the equations so we end up with A and F (i.e., pA and pF).

It would also be good to extend "where is the corresponding synaptic weight." to "where is the corresponding synaptic weight in mV."

The units should be consistent now, please check.

Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

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

@ackurth Nice, thanks!

@heplesser
Copy link
Contributor

@ackurth There seems to be a line break missing, making the copyright-checker unhappy:

[COPY] models/iaf_psc_delta.h: expected ' *  iaf_psc_delta.h', found ' *  iaf_psc_delta.h This file is part of NEST.'.

models/iaf_psc_delta.h Outdated Show resolved Hide resolved
models/iaf_psc_delta.h Show resolved Hide resolved
models/iaf_psc_delta.h Outdated Show resolved Hide resolved
models/iaf_psc_delta.h Outdated Show resolved Hide resolved
models/iaf_psc_delta.h Show resolved Hide resolved
models/iaf_psc_delta.h Show resolved Hide resolved
models/iaf_psc_delta.h Show resolved Hide resolved
models/iaf_psc_delta.h Outdated Show resolved Hide resolved
models/iaf_psc_delta.h Outdated Show resolved Hide resolved
models/iaf_psc_delta.h Show resolved Hide resolved
ackurth and others added 4 commits February 22, 2024 09:43
Co-authored-by: clinssen <c.linssen@fz-juelich.de>
Co-authored-by: clinssen <c.linssen@fz-juelich.de>
Co-authored-by: clinssen <c.linssen@fz-juelich.de>
Co-authored-by: clinssen <c.linssen@fz-juelich.de>
Co-authored-by: clinssen <c.linssen@fz-juelich.de>
@heplesser heplesser requested a review from clinssen March 1, 2024 15:04
Copy link
Contributor

@clinssen clinssen left a comment

Choose a reason for hiding this comment

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

OK, this looks fine. Many thanks!

@jessica-mitchell jessica-mitchell merged commit 6e58d59 into nest:master Mar 4, 2024
24 checks passed
Documentation automation moved this from Review to Done Mar 4, 2024
Models automation moved this from Review to Done Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Enhancement New functionality, model or documentation
Projects
Documentation
  
Done
Models
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants