-
Notifications
You must be signed in to change notification settings - Fork 117
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
Increase max number of ions per compartment #3055
Conversation
… for bitvector comparisons
(deleted.) |
I see that the comment is deleted, but I did try using ASAN. Is it this line the issue refers to Line 784 in c918f6b
Not sure how to test if this becomes a problem since it would just cause a normal overwrite? |
The comment was incorrect and therefore deleted. I know 1000 is a special number and things are separated by "enough", where enough means 1000 (somehow somewhere). What I would do is try with more than 1000 species, just to be sure. Then it would also be good to integrate the test into the automated test suite. With some luck you can put the MOD files and python script in the directory Lines 321 to 324 in c918f6b
|
@wthun I noticed that the PR if from The original PR seems like a good idea. NRN moves a little slower than small software projects; and there's vacations going on. It'll need review from others as well, and I was hoping one could set up some additional testing (possibly by someone from our side). To keep unrelated changes separate it would be helpful if you could create a PR from a dedicated branch into our master. Naturally, you can merge those branches into your copy of NRN which you can use if/until NRN has the features you need for your research. Edit: the new removed changes seem interesting too. (In a separate PR.) |
Thanks! Yes, I mixed up my directories, fixing this. |
fc84078
to
5179cc4
Compare
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 believe this change is sound but needs a test for interactions with some other parts of the code when an ion with mechanism index > 1000 is actually used. I'll add this test in a branch off this PR.
@wthun thank you for a very nice contribution. In #3081, @nrnhines developed a test that shows that there will be an issue with the magic 1000. We understand that there's interesting science that requires more Ions and would like to work towards enabling that usecase. However, it feels non-trivial. I'd like to propose the following:
@wthun is there a value of |
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.
Very nice refactoring!
In order to format NEURON code run:
from the root directory. This will download the required version of all formatters in a venv called |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3055 +/- ##
==========================================
- Coverage 67.40% 67.39% -0.02%
==========================================
Files 573 573
Lines 104927 104927
==========================================
- Hits 70729 70716 -13
- Misses 34198 34211 +13 ☔ View full report in Codecov by Sentry. |
Co-authored-by: Luc Grosheintz <[email protected]>
Co-authored-by: Luc Grosheintz <[email protected]>
Co-authored-by: Luc Grosheintz <[email protected]>
Co-authored-by: Luc Grosheintz <[email protected]>
I agree with @1uc but speculate that in the end the elimination of the magic number 1000 will be straightforward (but tedious) with a natural limit on number of mechanisms of maxint/2 just by changing the coding of semantics for the two ion cases. |
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.
Will deal with the magic number 1000 in a later PR along with additional tests.
@1uc Sounds good! I ran the formatting command and set the number of ions to 256. Our (@Hjorthmedh) use case had >60 ions/species (per compartment) for the D1 and M4 signal transduction cascades in striatal projection neurons, but this number could grow quickly when more receptors are included or different cascades are used for different cells, so maybe 256 would be a good margin? |
Quality Gate passedIssues Measures |
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.
Yes, 256 is unlikely to cause issues. If it allows you to progress, then that buys us some time to fix the other issues; and longer term will be able to allow many more ions.
I think I have more or less finished up the PR's that allow an unlimited number of types of ions that can exist within a compartment (#3081, #3089, #3097). However I am curious about how the ions are being used within your model. Are they used by mod files or exclusively used within in the RxD framework? (If more convenient, I'm happy to carry on this discussion via email ([email protected]). ) Behind my question is a speculation about whether RxD should/could provide some features that would avoid the fairly elaborate apparatus behind full ion mechanism behavior provided by the mod file USEION statement. |
@nrnhines We've initially represented signal transduction cascades within RxD, and their downstream effects by having membrane mechanisms read the concentration of e.g. PKA. Synaptic release to trigger the cascades also use .mod files along with include_flux from RxD to couple them. However, we've also considered representing the signal transduction cascades in mod files due to run time requirements. In our benchmarks, RxD brings a 10-20x slow down compared to having no cascade, while the same model represented as ODE:s in a mod file brings only a slow down of 1.45x. The latter didn't write the ions to the membrane, we only tested the run time required to compute the reaction by itself. The ODE representation also seem to scale linearly with the number of ODE:s, while the RxD representation seem to scale cubically with the number of species. Solving for the Jacobian essentially accounts for the full slow down in benchmarks. We've speculated that maybe we could generate nmodl code from our RxD specifixation (e.g. using the nmodl framework) but still harmonize with the rest of the RxD features. But maybe that's fairly tangential to this question? |
There's a relatively low limit (128) on the number of ions (and consequently RxD species) due to using ```long´´´ for bitvector comparisons. This PR extends this to 10000 (or an arbitrary number chosen at compile time) by using std::bitset.
This approach tried to minimize other changes. I guess there could be a dynamic limit by using vectors of booleans or the dynamic bitset from boost instead.
Rationale: Issue #2947 (comment)
Minimal test:
caldyn_ms.mod
sneaky_caldyn_ms.mod