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

nrn_setup: use std::vector for cell_groups (dat_files) #3185

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

ferdonline
Copy link
Member

@ferdonline ferdonline commented Nov 7, 2024

Context

Coreneuron nrn_setup uses a plain malloc'd array for holding the input data cell groups (from files.dat).
These arrays, besides being plain old and not self managed, could not grow and had a static size of nfiles/nranks + 1.

However, in the view of the upcoming load balanced input data, each rank may hold a different count of cell groups. Those changes will come as a follow-up PR.

This PR

Refactors this part of the code to replace the data structure by an std::vector, which

  • grows as required and auto deallocs
  • avoids the book-keeping of the size as a separate variable

@ferdonline ferdonline changed the title nrn_setup: use array for cell_groups (dat_files) nrn_setup: use std::vector for cell_groups (dat_files) Nov 7, 2024
@bbpbuildbot

This comment has been minimized.

@ferdonline ferdonline force-pushed the leite/dat_files_use_vec branch from c30f255 to d05e252 Compare November 7, 2024 18:57
Copy link

✔️ d05e252 -> Azure artifacts URL

Copy link

codecov bot commented Nov 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.06%. Comparing base (de07242) to head (1853c77).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3185      +/-   ##
==========================================
- Coverage   67.06%   67.06%   -0.01%     
==========================================
  Files         572      572              
  Lines      111073   111074       +1     
==========================================
  Hits        74488    74488              
- Misses      36585    36586       +1     

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

@bbpbuildbot

This comment has been minimized.

Copy link

✔️ 38baa26 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

@ferdonline ferdonline marked this pull request as ready for review November 8, 2024 01:28
@ferdonline ferdonline requested a review from 1uc November 8, 2024 01:28
src/coreneuron/io/nrn_setup.cpp Outdated Show resolved Hide resolved
src/coreneuron/io/nrn_setup.cpp Show resolved Hide resolved
src/coreneuron/io/nrn_setup.cpp Outdated Show resolved Hide resolved
src/coreneuron/io/user_params.hpp Show resolved Hide resolved
Copy link

✔️ 4e0e064 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

Copy link

✔️ 86be204 -> Azure artifacts URL

Copy link

✔️ 63a32f6 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

@ferdonline
Copy link
Member Author

@1uc anything missing here?

Copy link
Collaborator

@1uc 1uc left a comment

Choose a reason for hiding this comment

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

AFAICT, this is now a refactoring that adds more RAII and other improvements, but doesn't seem to have any functional change.

@ferdonline
Copy link
Member Author

Indeed, there should be no functional changes, including preallocating the vector with the same size as the original array

@bbpbuildbot

This comment has been minimized.

Copy link

✔️ 0f55dd6 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

Copy link

✔️ 1033a0c -> Azure artifacts URL

Copy link

sonarcloud bot commented Dec 10, 2024

@ferdonline ferdonline requested a review from alkino December 10, 2024 23:56
Copy link

✔️ 1853c77 -> Azure artifacts URL

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.

6 participants