Skip to content
This repository was archived by the owner on Mar 20, 2023. It is now read-only.

No more check parameters from argc / argv but from the parser. Fix #184#407

Merged
pramodk merged 3 commits into
masterfrom
fix_mpi_init
Oct 22, 2020
Merged

No more check parameters from argc / argv but from the parser. Fix #184#407
pramodk merged 3 commits into
masterfrom
fix_mpi_init

Conversation

@alkino

@alkino alkino commented Oct 22, 2020

Copy link
Copy Markdown
Member

We have to decide what to do with other arguments just like -p4 that is not valid with the new parser.

Does someone know what is nrn_music?

@alkino alkino linked an issue Oct 22, 2020 that may be closed by this pull request
-mpi still needed with --read-config #184
Closed
alexsavulescu
alexsavulescu reviewed Oct 22, 2020
View reviewed changes
Comment thread coreneuron/mpi/nrnmpi.cpp Outdated
@alexsavulescu

alexsavulescu commented Oct 22, 2020

Copy link
Copy Markdown
Contributor

Does someone know what is nrn_music?

I've seen this in NEURON, however I couldn't find any reference for its use. I believe it's a sort of NET events relayer. @nrnhines @pramodk would you know more about it?

@alkino

alkino commented Oct 22, 2020

Copy link
Copy Markdown
Member Author

Maybe I need to add || arg == "--mpi" because there is maybe someone / somewhere giving it args!

@pramodk

pramodk commented Oct 22, 2020

Copy link
Copy Markdown
Collaborator

Does someone know what is nrn_music?

Currently it's not but I wouldn't remove ( it could come back).

Maybe I need to add || arg == "--mpi" because there is maybe someone / somewhere giving it args!

@alkino : wanted to ask same thing because NEURON (and neurodamus) pass arguments via CLI.

@alkino
Remove nrn_music and simplify nrnmpi_init.
4dc9022 
From now, you should call nrnmpi_init only if you want to use mpi!
@alkino

alkino commented Oct 22, 2020

Copy link
Copy Markdown
Member Author

Endly remove all of this, as we think that main1.cpp is the only user of nrnmpi_init.

@alkino

alkino commented Oct 22, 2020

Copy link
Copy Markdown
Member Author

Pr. @nrnhines can we have a really quick and fast review of this?

Regarding callers of nrnmpi_init (only us?), and nrn_music (useless?).

Regards

@pramodk

pramodk commented Oct 22, 2020

Copy link
Copy Markdown
Collaborator

CoreNEURON will be either called by NEURON as library (where MPI is already initialised) or CoreNEURON will be launched independently. Hence extra options we had (e.g. always initialise MPI) are not necessary in my opinion.

Thanks for cleaning that up!

@alkino

alkino commented Oct 22, 2020

Copy link
Copy Markdown
Member Author

The base branch restricts merging to authorized users. I leave authorized users do.

iomaganaris
iomaganaris reviewed Oct 22, 2020
View reviewed changes

@iomaganaris iomaganaris left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Just a small nitpick

Comment thread coreneuron/mpi/nrnmpi.cpp Outdated
@pramodk pramodk merged commit 7c297eb into master Oct 22, 2020
@pramodk pramodk deleted the fix_mpi_init branch October 22, 2020 22:16
pramodk pushed a commit to neuronsimulator/nrn that referenced this pull request Nov 2, 2022
@alkino
No more check parameters from argc / argv but from the parser. Fix Bl…
f0066eb 
…ueBrain/CoreNeuron#184 (BlueBrain/CoreNeuron#407)

- no more check parameters from argc / argv but from the parser
- remove nrn_music and simplify nrnmpi_init.
- you should call nrnmpi_init only if you want to use mpi!

CoreNEURON Repo SHA: BlueBrain/CoreNeuron@7c297eb
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers
@alexsavulescu alexsavulescu alexsavulescu left review comments
@pramodk pramodk pramodk approved these changes
@iomaganaris iomaganaris iomaganaris approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

-mpi still needed with --read-config

4 participants

@alkino @alexsavulescu @pramodk @iomaganaris