no limit on number of ion mechanisms#3089
Conversation
… for bitvector comparisons
Co-authored-by: Luc Grosheintz <luc.grosheintz@gmail.com>
Co-authored-by: Luc Grosheintz <luc.grosheintz@gmail.com>
Co-authored-by: Luc Grosheintz <luc.grosheintz@gmail.com>
Co-authored-by: Luc Grosheintz <luc.grosheintz@gmail.com>
|
✔️ aea6299 -> Azure artifacts URL |
|
@nrnhines : gitlab ci can be ignored. As it’s going into branch, it can be merged. |
|
✔️ a9de3d7 -> Azure artifacts URL |
This comment has been minimized.
This comment has been minimized.
|
|
✔️ b608ddd -> Azure artifacts URL |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3089 +/- ##
==========================================
- Coverage 66.90% 66.89% -0.01%
==========================================
Files 572 572
Lines 106671 106662 -9
==========================================
- Hits 71364 71355 -9
Misses 35307 35307 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
This seems to solve the problem without a hard coded upper limit (that's less than 2^31).
It's very common in NEURON to have maps mech_type -> f(mech_type) and this follows that convention. In this case it might be easier to store inverse map, i.e. for each ionconc_type save the ids of the mechanisms that write to it.
In pseudo code it might look something like this:
static std::map<int, std::set<int>> ionconctype2writers;
const S& register_ion_write_access(int mechtype, int iontype, int side) {
auto& writers = ionconctype2writers[ionconctype];
writers.insert(mechtype);
return writers;
}
void nrn_check_conc_write(Prop* pmech, Prop* pion, int side) {
auto & writers = register_ion_write_access(pmech->_type, pion->_type, side);
// ...
if(ii && flag) {
// ...
for (p = pion->next; p; p = p->next) {
// ...
if(writers.count(mechtype) == 1) {
// ...
}
If you want you can then also short circuit the following loop:
for (p = pion->next; p; p = p->next)
whenever writers.size() == 1.
I was wondering about that. And the inverse seems conceptually better. But let me know if the following tips the scales in the direction I chose (perhaps I should have added a comment.) I expect a very large number of ions and a very small number of mechanisms (most often 1) that write to those ions. So is there a memory/performance benefit to map(mech_that_write) -> set(ionconc_written_by_that_mech) over the inverse? |
|
@nrnhines That seems like a good justification. Let's count the number of large searches in the case where one mechanism sets all ions:
In the reverse approach we can replace the Edit: I'd like to expand a bit. In the forward variation, if the outer map is large then we pay the cost for searching it once per mechanism registered at that node; and for every hit we need to also do a I suspect both approaches suffer if the |
I'm going to go ahead and merge this. I think it is very likely there is a meta-reason not to waste anymore time on this. |
PR 3055 extended the allowed number of ion mechanisms from 31 to
max_ions = 256;by replacingwith
This PR extension to #3081 makes the underlying alogorithm more understandable by using:
Also, with the map of sets, the number of ions allowed in this context in now essentially unlimited. I.e., INT_MAX/2 or more than 1 billion.