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.

Sign up for GitHub

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

Jump to bottom

ENH: Refactor BatchSimulate Example and Improve Documentation #857

Merged
merged 12 commits into from
Dec 12, 2024
Merged

ENH: Refactor BatchSimulate Example and Improve Documentation #857

merged 12 commits into from
Dec 12, 2024

Conversation

samadpls
Copy link
Contributor

@samadpls samadpls commented Aug 10, 2024 •
edited
Loading

Adjusted parameter descriptions and added more informative explanations for waveforms and simulation outputs.

@samadpls samadpls force-pushed the feat/batch_simulation branch 2 times, most recently from 27fcbb6 to 86a68e8 Compare August 10, 2024 19:50
ntolley
ntolley reviewed Aug 12, 2024
View reviewed changes
hnn_core/batch_simulate.py Outdated Show resolved Hide resolved
@samadpls samadpls force-pushed the feat/batch_simulation branch 2 times, most recently from 719b17f to 455d5b7 Compare August 13, 2024 12:41
@samadpls samadpls changed the title ENH: BatchSimulate to Handle JSON File Paths ENH: BatchSimulate to Avoid Redundant Network Saving Aug 14, 2024
ntolley
ntolley reviewed Aug 15, 2024
View reviewed changes
hnn_core/batch_simulate.py Outdated Show resolved Hide resolved
@samadpls samadpls force-pushed the feat/batch_simulation branch 11 times, most recently from a0a1a47 to bb511df Compare August 17, 2024 20:12
@samadpls samadpls force-pushed the feat/batch_simulation branch from 5a30512 to 3366146 Compare August 20, 2024 11:29
@samadpls samadpls force-pushed the feat/batch_simulation branch from 3366146 to 2ede6ad Compare August 28, 2024 09:39
@samadpls samadpls changed the title ENH: BatchSimulate to Avoid Redundant Network Saving ENH: Refactor BatchSimulate Example and Improve Documentation Sep 10, 2024
@samadpls samadpls force-pushed the feat/batch_simulation branch from 2ede6ad to e1edb6c Compare September 10, 2024 01:54
@samadpls samadpls force-pushed the feat/batch_simulation branch from e1edb6c to 79e701e Compare September 24, 2024 16:15
@samadpls samadpls force-pushed the feat/batch_simulation branch from 79e701e to 9408061 Compare October 10, 2024 18:40
@samadpls samadpls force-pushed the feat/batch_simulation branch 3 times, most recently from 74b16cd to 671753a Compare November 26, 2024 20:48
@asoplata
Copy link
Collaborator

@samadpls just opened a PR on your branch: samadpls#5

I think the issue was just indexing on the start/stop positions of the batch weren't being calculated correctly. @samadpls @asoplata @dylansdaniels any ideas on how to test that batches are actually being run in parallel and not serially? Doesn't need to be super fancy as we should get this PR merged

I've been going through the Joblib docs, and unfortunately I'm not sure there's a trivial way to measure the number of active workers (or total workers at all) inside a simple Parallel call like we use in BatchSimulate.simulate_batch. For testing multiple simultaneous workers, we may have to wrap our simulate_batch call inside a block specific to each backend, such as https://github.com/joblib/loky?tab=readme-ov-file#usage . I'm assuming that, in that example, Loky would constrain the Parallel instantiation, but I'm not sure. It may actually be easier to mock simulate_batch and test that, instead of actually calling it.

@samadpls
Copy link
Contributor Author

@samadpls just opened a PR on your branch: samadpls#5

I think the issue was just indexing on the start/stop positions of the batch weren't being calculated correctly. @samadpls @asoplata @dylansdaniels any ideas on how to test that batches are actually being run in parallel and not serially? Doesn't need to be super fancy as we should get this PR merged

I have added timing around the start and end of simulate_batch to track execution duration, which should help us understand parallelism better. Let me know if you think we should take a different approach or add more specific checks.

@samadpls samadpls force-pushed the feat/batch_simulation branch 2 times, most recently from d1503e8 to 275129a Compare November 26, 2024 21:10
@asoplata asoplata mentioned this pull request Nov 26, 2024
Open
4 tasks
@samadpls samadpls force-pushed the feat/batch_simulation branch 2 times, most recently from 5648695 to c6ab1ae Compare November 26, 2024 21:20
@asoplata
Copy link
Collaborator

Thanks for reverting the joblib addition in _run_single_sim @samadpls and all your work with the debugging and testing! Since this PR is mostly about the example Howto and not about the BatchSimulation config API or its tests, I've made a separate issue here #955 for refactoring BatchSimulation's tests (I have not yet made an issue for the config API). Once the "dust has settled" with the commits and we've taken time to manually test this, it will probably be ready for merge soon.

ntolley
ntolley reviewed Dec 2, 2024
View reviewed changes
hnn_core/batch_simulate.py Outdated Show resolved Hide resolved
samadpls and others added 11 commits December 3, 2024 21:33
@samadpls
ENH BatchSimulate for JSON path handling
65dd021 
Signed-off-by: samadpls <abdulsamadsid1@gmail.com>
@samadpls
Fix: Docstring of dic_to_network
8c10d95 
Signed-off-by: samadpls <abdulsamadsid1@gmail.com>
@samadpls
ENH: Reorder the init params
dcea3fa 
Signed-off-by: samadpls <abdulsamadsid1@gmail.com>
@samadpls
Added record_isec vsec validation test
a565d2e 
Signed-off-by: samadpls <abdulsamadsid1@gmail.com>
@samadpls
Removed net_json param and update test
d12a0b5 
Signed-off-by: samadpls <abdulsamadsid1@gmail.com>
@samadpls @ntolley
Refactored Example and initialize parameters
11b50dd 
Co-authored-by: Nicholas Tolley <55253912+ntolley@users.noreply.github.com>
Signed-off-by: samadpls <abdulsamadsid1@gmail.com>
@samadpls @ntolley
Enh: visualization of dipole responses in plot_batch_simulate
fffcb17 
Co-authored-by: Nicholas Tolley <55253912+ntolley@users.noreply.github.com>
Signed-off-by: samadpls <abdulsamadsid1@gmail.com>
@samadpls
Refactor batch simulation parameters and backend
4759e37 
Signed-off-by: samadpls <abdulsamadsid1@gmail.com>
@ntolley @samadpls
[MRG] Fix indexing for batch simulations (#5)
a6114c9 
* ENH BatchSimulate for JSON path handling

Signed-off-by: samadpls <abdulsamadsid1@gmail.com>

* Fix: Docstring of dic_to_network

Signed-off-by: samadpls <abdulsamadsid1@gmail.com>

* ENH: Reorder the init params

Signed-off-by: samadpls <abdulsamadsid1@gmail.com>

* Added record_isec vsec validation test

Signed-off-by: samadpls <abdulsamadsid1@gmail.com>

* Removed net_json param and update test

Signed-off-by: samadpls <abdulsamadsid1@gmail.com>

* Refactored  Example and initialize parameters

Co-authored-by: Nicholas Tolley <55253912+ntolley@users.noreply.github.com>
Signed-off-by: samadpls <abdulsamadsid1@gmail.com>

* Enh: visualization of dipole responses in plot_batch_simulate

Co-authored-by: Nicholas Tolley <55253912+ntolley@users.noreply.github.com>
Signed-off-by: samadpls <abdulsamadsid1@gmail.com>

* Refactor batch simulation parameters and backend

Signed-off-by: samadpls <abdulsamadsid1@gmail.com>

* batches run in parallel

---------

Signed-off-by: samadpls <abdulsamadsid1@gmail.com>
Co-authored-by: samadpls <abdulsamadsid1@gmail.com>
@samadpls
Refactor: Removed joblib from simulate_dipole, and added parallel exe…
fb0b79e 
…cution test.

Signed-off-by: samadpls <abdulsamadsid1@gmail.com>
@ntolley @samadpls
Refactor: Simplify BatchSimulate parameters by removing n_jobs
d014b3f 
Signed-off-by: samadpls <abdulsamadsid1@gmail.com>
@samadpls samadpls force-pushed the feat/batch_simulation branch from 7c13389 to d014b3f Compare December 3, 2024 16:33
asoplata
asoplata requested changes Dec 10, 2024
View reviewed changes
Copy link
Collaborator

@asoplata asoplata left a comment

Choose a reason for hiding this comment

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

I did some testing with this and I think it needs only some very minor changes. Once these are done, I would be willing to merge it. Thanks @samadpls !

hnn_core/batch_simulate.py Outdated Show resolved Hide resolved
examples/howto/plot_batch_simulate.py Outdated Show resolved Hide resolved
hnn_core/batch_simulate.py Show resolved Hide resolved
@samadpls
Remove unused Dask code and simplify BatchSimulate initialization
a984e84 
Signed-off-by: samadpls <abdulsamadsid1@gmail.com>
@samadpls samadpls requested a review from asoplata December 12, 2024 13:54
asoplata
asoplata approved these changes Dec 12, 2024
View reviewed changes
Copy link
Collaborator

@asoplata asoplata left a comment

Choose a reason for hiding this comment

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

Retested with new changes, and it's ready to go!

@asoplata asoplata merged commit c374c6a into jonescompneurolab:master Dec 12, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ntolley ntolley ntolley left review comments

@asoplata asoplata asoplata approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@samadpls @ntolley @asoplata @jasmainak