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

Allow API configuration to preserve the axons during setup #155

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ferdonline
Copy link
Collaborator

@ferdonline ferdonline commented Apr 18, 2024

Context

With BlueConfig one could ask a circuit to preserve the axons in full detail via a config entry DetailedAxons.

However, that options was never made globally available via a flag, which enables it also for Sonata simulations.

This PR covers that.

Scope

  • Introduce the new Neurodamus top-level option keep_axon
  • Propagate to all Detailed neuron circuits which don't set the option explicitly.

Testing

in the testing environment under test/ngv_wip with the patched emodels_atp_1p4 one must see

Script:

nd = neurodamus.Neurodamus(
    "simulation_config.json",
    keep_axon=True,
    auto_init=False,
    cleanup_atexit=False,
    logging_level=2,
)
nd.init()
nd.run()

Output:

[VERB]  -> Keeping axons ENABLED
...
[INFO] Executing actions after stdinit...
[INFO] Now deleting the axon!
REPLACING AXON!
[INFO] Now deleting the axon!
REPLACING AXON!

Review

  • PR description is complete
  • Coding style (imports, function length, New functions, classes or files) are good
  • Unit/Scientific test added
  • Updated Readme, in-code, developer documentation

@bbpbuildbot

This comment has been minimized.

@cattabiani
Copy link

I wrote here my findings:

https://bbpteam.epfl.ch/project/issues/browse/BBPBGLIB-984.

Let me know if you prefer the reports directly here instead

@WeinaJi WeinaJi marked this pull request as ready for review May 13, 2024 11:32
@WeinaJi WeinaJi force-pushed the leite/propagate_cli_keep_axon_to_circuits branch from d8bbb28 to ed80d0b Compare May 13, 2024 11:32
@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

Copy link
Collaborator

@WeinaJi WeinaJi left a comment

Choose a reason for hiding this comment

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

LGTM!
Just one question: is there any need to add a CLI option for this parameter?

@cattabiani
Copy link

not from me

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot
Copy link

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.

None yet

5 participants