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

Can we rename nocmodl generated file to .cpp instead of .c? #384

Closed
pramodk opened this issue Jan 1, 2020 · 7 comments
Closed

Can we rename nocmodl generated file to .cpp instead of .c? #384

pramodk opened this issue Jan 1, 2020 · 7 comments
Assignees

Comments

@pramodk
Copy link
Member

pramodk commented Jan 1, 2020

I am creating this issue as question (/proposal):

What if we merely rename nocmodl generated file .cpp instead of .c?

Would this be a problem?

  • As C++ is backward compatible with C (almost!), we don't expect MAJOR problem with such change (see this and this).
  • People can still use their models as it is. nrnivmodl will compile all generated files with CXX instead of CC. Rest remain same.

Well, one can come up with cases which compile with CC but not using CXX. But I think changes required are always result into better code. This can be discussed / debated in detail based on our models in ModelDB.

What will be benefit?

  • One of the major benefit will be VERBATIM blocks:
    • we will be able to use C++ instead of C in verbatim blocks. See example of this MOD file where memory management became so convoluted. If we can use C++ (e.g. vectors), it will become so much simple.
  • even though we want to avoid VEBATIM blocks, today we often need them to interface with external libraries. if we can write (modern) C++ code, I believe such MOD files can become much more compact and nicer.
  • when we interface with any C++ library (e.g. SONATA), we have to always create new C bindings to use with MOD files. With .cpp file, this will be avoided.

cc : @ohm314 @ramcdougal

@nrnhines
Copy link
Member

nrnhines commented Jan 1, 2020

In principle I have no objection to this. It would be a good idea to test this with all the NEURON ModelDB models (the current nrnverify scripts have not been kept to the point of complete coverage.)
But I would not be surprised if not a single change to a VERBATIM block was needed after the transformation. The only halfway annoying thing is that I will have to augment the minimal development environment distributed with Windows to allow compiling c++.

@ramcdougal
Copy link
Member

I agree that switching to C++ has minimal downside and introduces significant new features.

That said, is this solving the wrong problem?

In my mind, MOD files are for specifying ion channel (etc) kinetics.

The ingenious thing about MOD files is that they allow arbitrary code in VERBATIM blocks, but I'd argue that anyone using VERBATIM blocks is working around something that they perceive to be a limitation in NEURON/CoreNEURON.

In particular, if VERBATIM blocks are being used to interconnect NEURON with external tools, maybe we should just have a NEURON API for doing that. For NEURON, ctypes and cvode.extra_scatter_gather can probably do most of this as-is and examples might be all that's needed; for CoreNEURON, things are probably trickier.

For the adex.mod example, should we think of that as an implicit feature request for NMODL? Shouldn't we view resorting to C as a sign of a limitation in the language? This is slightly complicated by the fact that there are now multiple NMODL compilers.

Models that avoid VERBATIM blocks have greater potential for being machine analyzed (in the sense of comparing models and not the model outputs). Perhaps a NEURON board should have an NMODL subcommittee? Or maybe that should be a stand-alone board, now that Arbor and others use NMODL?

@nrnhines
Copy link
Member

nrnhines commented Jan 2, 2020

A NEURON API is a necessary addition to the environment. Most of the labor is the documentation.
But I don't think it impacts the decision about nmodl emitting c++ instead of c as a way station to
eventual replacement by the new nmodl (which awaits another tedious implementation of output layer for NEURON). This in no way affects existing mod files. Other's use NMODL but generally with their own implementation of the translator. I can imagine people using the NEURON API in a mod file just to get the boilerplate interface to Python and Hoc.

@nrnhines
Copy link
Member

nrnhines commented Jan 2, 2020

CoreNEURON recently underwent a refactoring where all c files were renamed cpp and everything is
in the namespace coreneuron
that revealed a lot of opportunities to simplify code using stl. Perhaps that is still distant for NEURON.

@ohm314
Copy link
Member

ohm314 commented Jan 2, 2020

I think @pramodk's arguments make sense and I am in favor of this change. @ramcdougal is right in saying that we're trying to solve the wrong problem. But unfortunately I don't see a way how nmodl can become anytime soon a language powerful enough to encompass all the use-cases we currently need VERBATIM for. VERBATIM is to me a workaround to quickly extend MOD files with additional functionalities not available in the language itself. At the same time, we should take a look at the NEURON API (and NMODL language) and see what can be added to support some of those use-cases that VERBATIM is used for (e.g. asserts, prints seem like low-hanging fruits to me).

@pramodk
Copy link
Member Author

pramodk commented Jan 3, 2020

That said, is this solving the wrong problem?
In my mind, MOD files are for specifying ion channel (etc) kinetics.

@ramcdougal is right in saying that we're trying to solve the wrong problem.

I agree with above. I tried to use simplifying VERBATIM blocks as motivation for this ticket. Ideally Removing need of VERBATIM blocks should be the goal (which will be hard? :) )

For the adex.mod example, should we think of that as an implicit feature request for NMODL? Shouldn't we view resorting to C as a sign of a limitation in the language?

Agree as well! (it was wrong example for this feature)

Perhaps a NEURON board should have an NMODL subcommittee? Or maybe that should be a stand-alone board, now that Arbor and others use NMODL?

That's good idea! During last hackathon me/Omar were thinking of having a dedicated discussion about NMODL aspects but dropped it because of time constant. For upcoming hackathon, we can include this!

At the same time, we should take a look at the NEURON API (and NMODL language) and see what can be added to support some of those use-cases that VERBATIM is used

Yes. We already have analysed all VERBATIM blocks from ModelDB (there is Verbatim visitor in NMODL). Lets review those during next hackathon.

pramodk added a commit that referenced this issue Jan 5, 2020
  - nrnivmodl accept extra option -cpp by which all mod files
    are translated to .cpp instead of .c
  - all files are then compiled with CXX and relevant compiler
    flags to build special
  - keep everything backward compatible i.e. if -cpp flag is not
    provided then keep old behaviour
  - nocmodl accepts extra comamnd line argument ext=cpp (last argument)
    by which .mod file is translated to .cpp
  - nrnivmodl and relevant makefiles are updated to accept -cpp flag

fixes #384
pramodk added a commit that referenced this issue Aug 16, 2020
  - nrnivmodl accept extra option -cpp by which all mod files
    are translated to .cpp instead of .c
  - all files are then compiled with CXX and relevant compiler
    flags to build special
  - keep everything backward compatible i.e. if -cpp flag is not
    provided then keep old behaviour
  - nocmodl accepts extra comamnd line argument ext=cpp (last argument)
    by which .mod file is translated to .cpp
  - nrnivmodl and relevant makefiles are updated to accept -cpp flag

fixes #384
@pramodk
Copy link
Member Author

pramodk commented May 10, 2022

Answer of this ticket is yes and this is already being addressed in #708.

@pramodk pramodk closed this as completed May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants