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

[WIP] - Simulation updates #336

Merged
merged 33 commits into from
Nov 26, 2024
Merged

[WIP] - Simulation updates #336

merged 33 commits into from
Nov 26, 2024

Conversation

TomDonoghue
Copy link
Member

@TomDonoghue TomDonoghue commented Sep 7, 2024

This is an update through the new simulation functionality added in #329

Most of the updates are cleaning up and improving consistency across the sim module. This includes moving some functions around and re-organization sim sub-modules. A notable update is the adding simulation I/O functionality, so that simulations can be saved and loaded.

@neurodsp-tools neurodsp-tools deleted a comment from codecov bot Sep 7, 2024
@neurodsp-tools neurodsp-tools deleted a comment from codecov bot Sep 8, 2024
@neurodsp-tools neurodsp-tools deleted a comment from codecov bot Sep 8, 2024
@neurodsp-tools neurodsp-tools deleted a comment from codecov bot Sep 9, 2024
@neurodsp-tools neurodsp-tools deleted a comment from codecov bot Sep 10, 2024
Copy link
Member

@ryanhammonds ryanhammonds left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in reviewing this!

I read through everything here, except for the tests. I think this and the last, related PR, will be useful for running parameter grids of simulations! I left minor comments but everything looked good at a high level.

I was working on something similar here a while ago. The approach here accomplishes the same. I like that you've added parameter sampling too.

The only other thing, that doesn't necessarily need to be addressed here but maybe for something in the future, is parallelization over grids. I think here iterates over sets of parameters serially, maybe a multiprocessing pool could be used if scaling is a concern.

neurodsp/filt/filter.py Show resolved Hide resolved
neurodsp/sim/aperiodic.py Show resolved Hide resolved
tutorials/sim/plot_07_SimMulti.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Nov 26, 2024

Codecov Report

Attention: Patch coverage is 98.26690% with 10 lines in your changes missing coverage. Please review.

Project coverage is 98.24%. Comparing base (e494f93) to head (e1f186f).
Report is 36 commits behind head on main.

Files with missing lines Patch % Lines
neurodsp/sim/generators.py 83.33% 3 Missing ⚠️
neurodsp/sim/io.py 95.94% 3 Missing ⚠️
neurodsp/sim/params.py 98.07% 1 Missing ⚠️
neurodsp/sim/signals.py 98.30% 1 Missing ⚠️
neurodsp/sim/utils.py 0.00% 1 Missing ⚠️
neurodsp/utils/checks.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #336      +/-   ##
==========================================
+ Coverage   98.02%   98.24%   +0.22%     
==========================================
  Files         110      115       +5     
  Lines        3995     4327     +332     
==========================================
+ Hits         3916     4251     +335     
+ Misses         79       76       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@TomDonoghue
Copy link
Member Author

@ryanhammonds - thanks for checking in on this!

I should have totally had a look what was available in ndspflow - it totally slipped my mind to do so! At some point we should work through what is available where and consider consolidating / linking out to other implementations etc.

Also agreed on adding parallel support - I think that would be a great extension!

For now, I'll merge this in!

@TomDonoghue TomDonoghue merged commit c3f9c0e into main Nov 26, 2024
9 checks passed
@TomDonoghue TomDonoghue deleted the sim2 branch November 26, 2024 21:37
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.

2 participants