Skip to content

Fix Izhikevich model equation in the documentation#3220

Merged
jessica-mitchell merged 11 commits intonest:masterfrom
egorus1:master
Jun 20, 2024
Merged

Fix Izhikevich model equation in the documentation#3220
jessica-mitchell merged 11 commits intonest:masterfrom
egorus1:master

Conversation

@egorus1
Copy link
Contributor

@egorus1 egorus1 commented Jun 8, 2024

Solved issue about not rendered equation on the following page: https://nest-simulator.readthedocs.io/en/latest/models/izhikevich.html

Also I centered it, to make it look better with another equation.

@jessica-mitchell jessica-mitchell changed the title First equation can be rendered now on Izhikevich model doc page Fix Izhikevich model equation on doc page Jun 10, 2024
@jessica-mitchell jessica-mitchell added S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation. I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Jun 10, 2024
@jessica-mitchell jessica-mitchell self-requested a review June 10, 2024 09:02
@jessica-mitchell
Copy link
Contributor

Fixes #3219

@clinssen
Copy link
Contributor

Could you make the variable naming of u/U_m consistent? Cheers!

@egorus1
Copy link
Contributor Author

egorus1 commented Jun 10, 2024

Could you make the variable naming of u/U_m consistent? Cheers!

@clinssen Are you talking about these lines of code?

double u_; // membrane recovery variable

@clinssen
Copy link
Contributor

The alignment should probably be done globally via CSS or other options instead; entering spaces like this is a bit too ad-hoc and can break things in the future.

Re the naming, see

V_m mV Membrane potential
and the line after it.

@egorus1
Copy link
Contributor Author

egorus1 commented Jun 11, 2024

Hi! I made changes, let me know if it's okay now)

@jessica-mitchell
Copy link
Contributor

@clinssen I was looking at the Izhikevich model in nestml, and noticed the use of v and u in the first equation but in NEST we used V_m and u (except there is a v in the last line of that second part of the equation).

Is v equivalent to V_m? and should u stay or should it be U_m?

Copy link
Contributor

@jessica-mitchell jessica-mitchell left a comment

Choose a reason for hiding this comment

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

hi @PrabhuUdurg after reviewing with @pnbabu, we realize that the u should be U_m and any case of v should be V_m. I made some suggestions but could you please check the page if there are any cases of u or v that still need to be updated. Thanks!

@egorus1
Copy link
Contributor Author

egorus1 commented Jun 19, 2024

@jessica-mitchell I will change it and submit new pull request, thanks!

Copy link
Contributor

@jessica-mitchell jessica-mitchell left a comment

Choose a reason for hiding this comment

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

@PrabhuUdurg Can you also update lines 59 - 68, where u and v are mentioned and change them to the correct ones?
Also, can you merge master - we had an update that fixes a test and our CI pipeline won't pass unless your branch is up-to-date with master.

@egorus1
Copy link
Contributor Author

egorus1 commented Jun 20, 2024

@jessica-mitchell Should v on line 60 be changed to V_m too?

@jessica-mitchell
Copy link
Contributor

@jessica-mitchell Should v on line 60 be changed to V_m too?

yep

Copy link
Contributor

@jessica-mitchell jessica-mitchell left a comment

Choose a reason for hiding this comment

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

Thanks @PrabhuUdurg looks good!

@jessica-mitchell jessica-mitchell merged commit e0ea2a8 into nest:master Jun 20, 2024
@egorus1
Copy link
Contributor Author

egorus1 commented Jun 20, 2024

Thanks @PrabhuUdurg looks good!

Thank you for assistance! My first merged request)

@terhorstd terhorstd changed the title Fix Izhikevich model equation on doc page Fix Izhikevich model equation in the documentation Jul 8, 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: Maintenance Work to keep up the quality of the code and documentation.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants

Comments