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

Add EI_clustered_network by Rostami et al to PyNEST examples #3028

Merged
merged 25 commits into from
Apr 10, 2024

Conversation

schmitfe
Copy link
Contributor

@schmitfe schmitfe commented Dec 4, 2023

We added a PyNEST example to simulate an EI-clustered network 1. The example has a similar structure as Potjans_2014 PyNEST example. We provide a short readme and documented source code.
After introduction of the high-level routines to create clustered connectivity, this example might be a good candidate to use them.

Footnotes

  1. Excitatory and inhibitory motor cortical clusters account for balance, variability, and task performance
    Vahid Rostami, Thomas Rost, Alexa Riehle, Sacha J. van Albada, Martin P. Nawrot
    bioRxiv 2020.02.27.968339; doi: https://doi.org/10.1101/2020.02.27.968339

@jessica-mitchell jessica-mitchell added T: Enhancement New functionality, model or documentation S: Normal Handle this with default priority labels Dec 5, 2023
@terhorstd terhorstd added the I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) label Dec 15, 2023
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 thanks for your contribution! I have a few changes requested along with a Pull request made against your branch to add the example to the index page in the docs.

pynest/examples/EI_clustered_network/default_parameters.py Outdated Show resolved Hide resolved
pynest/examples/EI_clustered_network/Network_Schematic.png Outdated Show resolved Hide resolved
pynest/examples/EI_clustered_network/helper.py Outdated Show resolved Hide resolved
pynest/examples/EI_clustered_network/README.rst Outdated Show resolved Hide resolved
pynest/examples/EI_clustered_network/README.rst Outdated Show resolved Hide resolved
pynest/examples/EI_clustered_network/README.rst Outdated Show resolved Hide resolved
pynest/examples/EI_clustered_network/README.rst Outdated Show resolved Hide resolved
pynest/examples/EI_clustered_network/README.rst Outdated Show resolved Hide resolved
pynest/examples/EI_clustered_network/network.py Outdated Show resolved Hide resolved
pynest/examples/EI_clustered_network/network.py Outdated Show resolved Hide resolved
pynest/examples/EI_clustered_network/network.py Outdated Show resolved Hide resolved
pynest/examples/EI_clustered_network/network.py Outdated Show resolved Hide resolved
@jessica-mitchell
Copy link
Contributor

@schmitfe thanks for the updates, can you fix the conflicts in the CI please? We encourage the developers to set up a pre-commit hook that will check isort/black etc automatically when a commit is made. Take a look at our developer tools for info on that.

@schmitfe
Copy link
Contributor Author

schmitfe commented Mar 21, 2024

@heplesser I was able to change the raster_plot.py to include color groups and think the changes did not introduce any changes to its behavior if no colorgroups are provided. But the change might still need some discussion and refinement.
Should I create another pull request with the change of the raster_plot or an issue, or just include it within this pull request?

Right now I put it into a new branch:
https://github.com/schmitfe/nest-simulator/tree/add_color_groups_raster_plot
There I changed the raster_plot.py and added brunel_alpha_nest_raster_color_groups.py in the examples which uses the new functionality. The current version is quite slow if color groups are used in larger networks. I was also not sure if it makes sense to change also the behavior for the histogram in this case to only contain actual plotted neuron IDs and/or split into one for each color group.

@schmitfe schmitfe requested a review from heplesser March 21, 2024 16:19
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.

@schmitfe Thank you very much for the very thorough revision of the example. It looks good to me now, I just have a few suggestions on details.

@jessica-mitchell Could you take a look as well?

doc/htmldoc/examples/index.rst Outdated Show resolved Hide resolved
pynest/examples/EI_clustered_network/README.rst Outdated Show resolved Hide resolved
pynest/examples/EI_clustered_network/helper.py Outdated Show resolved Hide resolved
pynest/examples/EI_clustered_network/helper.py Outdated Show resolved Hide resolved
pynest/examples/EI_clustered_network/network_params.py Outdated Show resolved Hide resolved
pynest/examples/EI_clustered_network/network.py Outdated Show resolved Hide resolved
pynest/examples/EI_clustered_network/network.py Outdated Show resolved Hide resolved
pynest/examples/EI_clustered_network/network.py Outdated Show resolved Hide resolved
pynest/examples/EI_clustered_network/network.py Outdated Show resolved Hide resolved
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 for your contribution @schmitfe ! I have one small change and ask that you resolve the conflict, but otherwise it looks good. The example runs on latest EBRAINS release too.

pynest/examples/EI_clustered_network/README.rst Outdated Show resolved Hide resolved
@schmitfe
Copy link
Contributor Author

schmitfe commented Apr 9, 2024

Thanks for your contribution @schmitfe ! I have one small change and ask that you resolve the conflict, but otherwise it looks good. The example runs on latest EBRAINS release too.

Thanks for checking. I just corrected the wrong copyright header. Now all the checks should pass.

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.

@schmitfe Thanks a lot, all fine now, just the tests need to go through and we are good to merge.

@heplesser heplesser changed the title Add EI_clustered_network to PyNEST examples Add EI_clustered_network by Rostami et al to PyNEST examples Apr 10, 2024
@heplesser heplesser merged commit 56c720c into nest:master Apr 10, 2024
24 checks passed
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
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants