Merged
Conversation
RaulSimpetru
approved these changes
Jul 26, 2025
Member
RaulSimpetru
left a comment
RaulSimpetru
left a comment
There was a problem hiding this comment.
Hi @rnwatanabe,
Thanks a lot for the code you provided!
I’ve reformatted it a bit and bumped the version to 0.2.0.
Feel free to merge it whenever you're ready.
RaulSimpetru
added a commit
that referenced
this pull request
Apr 18, 2026
…tor accessor Addresses Reviewer #1 Code Review 1 and 2 (NCOMMS-26-006203-T). The previous design bound RANDOM_GENERATOR and SEED at module import time via `from myogen import RANDOM_GENERATOR, SEED`. Python's from-import copies the current binding into the importing module's namespace — so when set_random_seed() later rebinds myogen.RANDOM_GENERATOR, existing importers keep their stale reference. The SEED constant was never updated at all, so seeds derived from it (cells.py Cython Mersenne seeds, motor_unit_sim.py KMeans random_state) were frozen to 180319 for the life of the process. Fix: introduce get_random_generator() and get_random_seed() accessors in myogen/__init__.py. A function lookup always reads the current module global, so subsequent set_random_seed() calls propagate correctly. The RANDOM_GENERATOR and SEED module attributes still resolve via a module- level __getattr__ that emits DeprecationWarning and returns the current state, preserving backwards compatibility for external callers. Updated sites: - myogen/simulator/neuron/network.py (5 RNG usages) - myogen/simulator/neuron/cells.py (3 SEED-derived Cython seeds + 1 RNG draw) - myogen/simulator/core/muscle/muscle.py (3 RNG usages) - myogen/simulator/core/emg/surface/surface_emg.py (3 RNG usages) - myogen/simulator/core/emg/intramuscular/motor_unit_sim.py (1 KMeans random_state, 2 rng=RANDOM_GENERATOR caches, 1 draw) - myogen/simulator/core/emg/intramuscular/intramuscular_emg.py (1 RNG draw) Cell-specific seeds now use `get_random_seed() + (class_id+1)*(global_id+1)` — same mathematical structure as before, but the base seed tracks set_random_seed(). Tests: added tests/test_determinism.py with 8 regression tests covering byte-level reproducibility, seed propagation to downstream seed-derivation sites, and DeprecationWarning emission for the legacy attribute names. Added pytest>=8.0 to the dev dependency group. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
RaulSimpetru
added a commit
that referenced
this pull request
Apr 18, 2026
Addresses Reviewer #1 Code Review 3 (NCOMMS-26-006203-T). np.std with ddof=1 is undefined for n<2 (division by n-1 = 0) and returns NaN, which could silently corrupt downstream ensemble-statistics analysis when a pool has exactly one active unit after firing-rate filtering. Guards the sample-std branch in myogen/utils/helper.py:175 to return 0.0 when fewer than two units are active. The empty-population path (n=0) already returned 0.0 and is unchanged. Adds tests/test_firing_rate_statistics.py with three cases: - single active unit -> FR_std is 0.0 (regression for CR3) - empty population -> FR_std stays 0.0 - two distinct rates -> FR_std is finite and positive (sanity) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
RaulSimpetru
added a commit
that referenced
this pull request
Apr 18, 2026
Addresses Reviewer #1 Code Review 4 (NCOMMS-26-006203-T). 1) elephant is now a proper optional extra. - Removed elephant>=1.1.1 from [project.dependencies]. - Added elephant = ["elephant>=1.1.1"] to [project.optional-dependencies]. - The five code sites that import elephant (myogen/utils/helper.py, surface_emg.py, intramuscular_emg.py, force_model.py, force_model_vectorized.py) already wrap the import in a try/except ImportError and print a helpful "Install with: pip install myogen[elephant]" message when missing; those guards now match reality. 2) NEURON version unified to 8.2.7 in the Windows install docs. - README.md lines 50, 73: bumped 8.2.6 -> 8.2.7 (both installer link and version label). - docs/source/README.md line 42 and docs/source/index.md lines 50, 73: same bump. - Corrected the Python-version stub in the Windows installer filename to match the 8.2.7 release: py-39-310-311-312-313 (8.2.7 dropped 3.8 and added 3.13 compared to 8.2.6's py-38-39-310-311-312). Verified via `gh api repos/neuronsimulator/nrn/releases/tags/8.2.7`. - pyproject.toml and the CI workflow already pinned 8.2.7 for Linux/macOS; no change needed there. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
RaulSimpetru
added a commit
that referenced
this pull request
Apr 18, 2026
…text Addresses Reviewer #1 Code Review 5 (NCOMMS-26-006203-T). - "extandable" -> "extensible" in README.md, docs/source/index.md, docs/source/README.md. - muscle.py docstring (two places) replaces the informal "as determined by no one" placeholders with concrete rationale: the 30 mm default length and the 0.25 max territory-to-muscle ratio are nominal starting points for the FDI; users are told to revisit them for other muscles. Broader repo-wide grep for common English typos (recieve, teh, seperate, occured, lenght, etc.) found no additional hits. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
RaulSimpetru
added a commit
that referenced
this pull request
Apr 19, 2026
…tor accessor Addresses Reviewer #1 Code Review 1 and 2. The previous design bound RANDOM_GENERATOR and SEED at module import time via `from myogen import RANDOM_GENERATOR, SEED`. Python's from-import copies the current binding into the importing module's namespace — so when set_random_seed() later rebinds myogen.RANDOM_GENERATOR, existing importers keep their stale reference. The SEED constant was never updated at all, so seeds derived from it (cells.py Cython Mersenne seeds, motor_unit_sim.py KMeans random_state) were frozen to 180319 for the life of the process. Fix: introduce get_random_generator() and get_random_seed() accessors in myogen/__init__.py. A function lookup always reads the current module global, so subsequent set_random_seed() calls propagate correctly. The RANDOM_GENERATOR and SEED module attributes still resolve via a module- level __getattr__ that emits DeprecationWarning and returns the current state, preserving backwards compatibility for external callers. Updated sites: - myogen/simulator/neuron/network.py (5 RNG usages) - myogen/simulator/neuron/cells.py (3 SEED-derived Cython seeds + 1 RNG draw) - myogen/simulator/core/muscle/muscle.py (3 RNG usages) - myogen/simulator/core/emg/surface/surface_emg.py (3 RNG usages) - myogen/simulator/core/emg/intramuscular/motor_unit_sim.py (1 KMeans random_state, 2 rng=RANDOM_GENERATOR caches, 1 draw) - myogen/simulator/core/emg/intramuscular/intramuscular_emg.py (1 RNG draw) Cell-specific seeds now use `get_random_seed() + (class_id+1)*(global_id+1)` — same mathematical structure as before, but the base seed tracks set_random_seed(). Tests: added tests/test_determinism.py with 8 regression tests covering byte-level reproducibility, seed propagation to downstream seed-derivation sites, and DeprecationWarning emission for the legacy attribute names. Added pytest>=8.0 to the dev dependency group.
RaulSimpetru
added a commit
that referenced
this pull request
Apr 19, 2026
Addresses Reviewer #1 Code Review 3. np.std with ddof=1 is undefined for n<2 (division by n-1 = 0) and returns NaN, which could silently corrupt downstream ensemble-statistics analysis when a pool has exactly one active unit after firing-rate filtering. Guards the sample-std branch in myogen/utils/helper.py:175 to return 0.0 when fewer than two units are active. The empty-population path (n=0) already returned 0.0 and is unchanged. Adds tests/test_firing_rate_statistics.py with three cases: - single active unit -> FR_std is 0.0 (regression for CR3) - empty population -> FR_std stays 0.0 - two distinct rates -> FR_std is finite and positive (sanity)
RaulSimpetru
added a commit
that referenced
this pull request
Apr 19, 2026
Addresses Reviewer #1 Code Review 4. 1) elephant is now a proper optional extra. - Removed elephant>=1.1.1 from [project.dependencies]. - Added elephant = ["elephant>=1.1.1"] to [project.optional-dependencies]. - The five code sites that import elephant (myogen/utils/helper.py, surface_emg.py, intramuscular_emg.py, force_model.py, force_model_vectorized.py) already wrap the import in a try/except ImportError and print a helpful "Install with: pip install myogen[elephant]" message when missing; those guards now match reality. 2) NEURON version unified to 8.2.7 in the Windows install docs. - README.md lines 50, 73: bumped 8.2.6 -> 8.2.7 (both installer link and version label). - docs/source/README.md line 42 and docs/source/index.md lines 50, 73: same bump. - Corrected the Python-version stub in the Windows installer filename to match the 8.2.7 release: py-39-310-311-312-313 (8.2.7 dropped 3.8 and added 3.13 compared to 8.2.6's py-38-39-310-311-312). Verified via `gh api repos/neuronsimulator/nrn/releases/tags/8.2.7`. - pyproject.toml and the CI workflow already pinned 8.2.7 for Linux/macOS; no change needed there.
RaulSimpetru
added a commit
that referenced
this pull request
Apr 19, 2026
…text Addresses Reviewer #1 Code Review 5. - "extandable" -> "extensible" in README.md, docs/source/index.md, docs/source/README.md. - muscle.py docstring (two places) replaces the informal "as determined by no one" placeholders with concrete rationale: the 30 mm default length and the 0.25 max territory-to-muscle ratio are nominal starting points for the FDI; users are told to revisit them for other muscles. Broader repo-wide grep for common English typos (recieve, teh, seperate, occured, lenght, etc.) found no additional hits.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Created example with cortical inputs (example 07). I had to make some small modifications on the spike_train file and the plot function to accept either a current or cortical inputs.