-
Notifications
You must be signed in to change notification settings - Fork 41
Improved Command Line with CLI11 #179
Conversation
|
I haven't looked at details but could you also update readme? (@iomaganaris also take a look when you time permits.) |
iomaganaris
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall nice PR 👍
I think I added all the comments I had until now. We can discuss anything if needed
|
Please retest |
…thread" This reverts commit f975ab2.
| mkdir test${TEST}dat | ||
| mkdir ${TEST} | ||
| mpirun -n ${MPI_RANKS} ./x86_64/special --mpi -c sim_time=100 test${TEST}.hoc | ||
| mpirun -n ${MPI_RANKS} ./x86_64/special -mpi -c sim_time=100 test${TEST}.hoc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this was already there, but mpiexec is guaranteed to exist in mpi implementations, not mpirun. Besides there were these CPU affinity problem with mpirun.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pramodk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. 2 small comments / questions.
Also merging engine and enginemech.h
ferdonline
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dont merge yet. FloatingPoint is a bad idea in preprocessor macros!
We cant do VERSION > 0.16
|
Do it à la boost. 0.16.0 => 001600 Easy to compare |
|
Exactly! But I left patch out, I dont expect that level of granularity to be required. But what do you think? |
|
|
||
| /// All-in-one initialization of mechanisms and solver | ||
| int solve_core(int argc, char** argv); | ||
| extern int solve_core(int argc, char** argv); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extern is apparently required in C99 mode 😫
ferdonline
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I dont have any objections
|
So let's go ;) |
* First integration of CLI11 in Coreneuron * CLI11 integrated as ubmodule * Fixed integration tests for new CLI * cn_parameters structure includes constructor and parse function * Removed paths from cmdline_interface tests. * Changed cn_par to corenrn_param to make it clearer * Now program doesn't print empty string parameters * More details on new CLI on README.md * Fixed integration tests for CLI and GPU * Added `--show` option to facilitate debugging. * Fix a free usage when variable is created with new * Adding CORENEURON_VERSION in engine.h and also merging engine and enginemech.h * Use combined int version Co-authored-by: Nicolas Cornu <[email protected]> Co-authored-by: Pramod Kumbhar <[email protected]> Co-authored-by: Fernando Pereira <[email protected]> CoreNEURON Repo SHA: BlueBrain/CoreNeuron@fd28045
Implemented the CLI11 as new command line parser as requested in #150 .