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

Make hh.mod configure-able at compile-time #3294

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JCGoran
Copy link
Collaborator

@JCGoran JCGoran commented Dec 17, 2024

This is one way to fix #3000. It basically makes hh.mod configure-able by CMake, so we can decide at compile-time what to do with the file depending on whether or not we are using CoreNEURON.

* introduce `NRN_SUPPORTED_KEYWORD` to differentiate whether the backend
  (NEURON or CoreNEURON) supports it
* rename `hh.mod` to `hh.mod.in`
* add `hh.mod` to .gitignore since it's still generated
Copy link

✔️ fce18df -> Azure artifacts URL

Copy link

codecov bot commented Dec 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.07%. Comparing base (8dfe5a3) to head (fce18df).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3294   +/-   ##
=======================================
  Coverage   67.07%   67.07%           
=======================================
  Files         571      571           
  Lines      111055   111055           
=======================================
  Hits        74487    74487           
  Misses      36568    36568           

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

: `GLOBAL minf` will be replaced with `RANGE minf` if CoreNEURON enabled
GLOBAL minf, hinf, ninf, mtau, htau, ntau
: set at compile-time to `GLOBAL` if using NEURON, `RANGE` if using CoreNEURON
@NRN_SUPPORTED_KEYWORD@ minf, hinf, ninf, mtau, htau, ntau
Copy link
Member

Choose a reason for hiding this comment

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

@JCGoran : technically, this is correct but I just want to ask @nrnhines if there is a "preference" to have hh.mod in usable format. E.g. if someone wants to copy that directly and compile it. Can't think of use case but just want to ask it in advance.

Copy link
Member

Choose a reason for hiding this comment

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

"preference"

I don't know. This usage (exploiting GLOBAL for efficiency to effectively implement a multiple value return from a function) predates threads. And then, with a lot of internal shenanigans, promoting them to thread variables to avoid races. And then (for GPU) just replacing with RANGE when there are many many threads. The whole thing contradicts the desire for description instead of programming and we have been working around the problem ever since.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@pramodk hence the "one way to fix" part 😅 Another option would be to move the mod files from src/nrnoc to a subdir, like src/nrnoc/modfiles, which has 2 additional subdirs, like neuron and coreneuron, and depending on whether CoreNEURON is used or not, we use the mod files from the corresponding subdir. The upside of that is that it avoids configure_file magic (and wouldn't require yet another cmake variable, which are already hard to keep track of as it is), so all the mod files are still usable on their own. The backend-agnostic mod files could reside in modfiles/neuron, with their modfiles/coreneuron counterparts just being symlinks to those, while hh.mod would need to have 2 versions, one for NEURON, and one for CoreNEURON.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've made an alternative implementation in #3295 if that one would be more appropriate.

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.

Update hh.mod for CoreNEURON compatibility changes hh.mod in place during build
4 participants