Skip to content

Test for PR 3055#3081

Merged
nrnhines merged 25 commits into
masterfrom
hines/ions
Sep 30, 2024
Merged

Test for PR 3055#3081
nrnhines merged 25 commits into
masterfrom
hines/ions

Conversation

@nrnhines

Copy link
Copy Markdown
Member

This tests use of high mechanism index ions and mechanisms for #3055
At the moment there is a segfault for nion = 1000 but the test succeeds for nion = 950.
This PR should be based on wthun:master but I don' t know how to do that.

@azure-pipelines

Copy link
Copy Markdown

✔️ 7ccc143 -> Azure artifacts URL

@nrnhines

Copy link
Copy Markdown
Member Author

On my machine (ubuntu 24.04 gcc 13.2.0) the test segfaults at the same point for gdb and address sanitizer. Ie frame 1 is at .../install/share/nrn/demo/release/x86_64/cabpump.cpp:794 which is cao = _ion_cao; in static void nrn_init and

#define _ion_cao *(_ml->dptr_field<0>(_iml))

Frame 0 is

#0  0x00007fffcef352d1 in neuron::cache::MechanismRange<17ul, 7ul>::dptr_field<0> (this=0x7fffef7ac830, instance=0)
    at /home/hines/neuron/ions/buildsani/install/include/neuron/cache/mechanism_range.hpp:144
144	        return m_pdata_ptrs[variable][m_dptr_offset + instance];
(gdb) p variable
$6 = 0
(gdb) p m_dptr_offset
$7 = 0
(gdb) p instance
$8 = 0
(gdb) p m_pdata_ptrs
$9 = (double * const * const *) 0x5060000fe3c0
(gdb) p m_pdata_ptrs[variable]
$10 = (double * const * const) 0x0
(gdb)

At the moment I have no idea why m_pdata_ptrs[variable] is NULL but I suppose it make sense to start thinking about the consequences of nrn/src/nrnoc/init.cpp

// xx_ion and #xx_ion will get values of type and type+1000 respectively
...
int dparam_semantics_to_int(std::string_view name) {
    ...
        bool const i{name[0] == '#'};
        ...
            return s->subtype + i * 1000;

I see that the magic number 1000 in the context of ions appears in about 28 places.

1uc and others added 9 commits September 20, 2024 08:48
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>
@azure-pipelines

Copy link
Copy Markdown

✔️ 3f5c418 -> Azure artifacts URL

@azure-pipelines

Copy link
Copy Markdown

✔️ da974c2 -> Azure artifacts URL

@azure-pipelines

Copy link
Copy Markdown

✔️ 3f213b0 -> Azure artifacts URL

@nrnhines

Copy link
Copy Markdown
Member Author

There does seem to be a bug in the newly merged to master #3055 which is revealed by the CI error for ubuntu-22.04 - cmake (-DNRN_ENABLE_CORENEURON=ON -DNRN_ENABLE_INTERVIEWS=OFF -DNMODL_SANITIZERS=undefinedundefined)

146/309 Test  #80: hoctests::test_many_ions_py ..............................................***Failed    2.11 sec
4194
../src/nrnoc/eion.cpp:433:27: runtime error: implicit conversion from type 'int' of value -2147483648 (32-bit, signed) to type 'unsigned long long' changed the value to 18446744071562067968 (64-bit, unsigned)
4195
    #0 0x7f2de318ee67 in nrn_check_conc_write(Prop*, Prop*, int) /home/runner/work/nrn/nrn/build/../src/nrnoc/eion.cpp:433:27

Line 433 is

            ion_bit_[j] = (1 << k);

and the value of k can now be much larger than 32 (or 64). The point of this code is to raise a warning if two or more mechanisms at the same location write to the same ion concentration (e.g. cai or cao).
I speculate that line 433 should be written as

ion_bit_[j].set(k);

Also shouldn't

           assert(k < max_ions);

be before the ion_bit_[j] is set.

Food for thought...
In retrospect, given my prehistoric comment in the original code

    /* an embarassing hack */
    /* up to 32 possible ions */

and the now forseeable memory use of a vector of thousands of bitsets that are thousands of bits long merely to check that there is no ion concentration write conflict after the model has been constructed (and it is to be expected there are only a few distinct mechanisms that write to the same ion concentration), it may be reasonable to use a different algorithm for checking for ion concentration write conflicts.

@azure-pipelines

Copy link
Copy Markdown

✔️ 23071ee -> Azure artifacts URL

@azure-pipelines

Copy link
Copy Markdown

✔️ 6604aab -> Azure artifacts URL

@azure-pipelines

Copy link
Copy Markdown

✔️ ebf26b1 -> Azure artifacts URL

@nrnhines

Copy link
Copy Markdown
Member Author

The NEURON Code Coverage / Code Coverage (pull_request) Failing after 33m CI exhibits a fairly consistent error.

88/355 Test  #88: hoctests::test_neurondemo_py .............................................***Failed    1.40 sec
6106
stderr: libgcov profiling error:/home/runner/work/nrn/nrn/build/share/nrn/demo/release/x86_64/mod_func.gcda:overwriting an existing profile data with a different timestamp

but I don't know how to fix or avoid it. I wonder if it has to do with the addition of another mod file to neurondemo but I believe this error was occurring before the addition of that file.

@azure-pipelines

Copy link
Copy Markdown

✔️ 06f9ace -> Azure artifacts URL

@nrnhines

Copy link
Copy Markdown
Member Author

Note about 06f9ace . On my machine Ubuntu 24.04 I have LCOV version 2.0-1 . With

$ cmake .. -G Ninja -DCMAKE_INSTALL_PREFIX=install -DNRN_ENABLE_RX3D=OFF -DCMAKE_BUILD_TYPE=Debug -DNRN_ENABLE_COVERAGE=ON -DNRN_COVERAGE_FILES="src/ivoc/apwindow.cpp"
$ ninja install

I experience

$ ninja cover_begin
...
geninfo: ERROR: /home/hines/neuron/nrn/src/ivoc/apwindow.cpp: unmatched LCOV_EXCL_START at line 413 - saw EOF while looking for matching LCOV_EXCL_STOP
	(use "geninfo --ignore-errors mismatch ..." to bypass this error)
Excluded data for 4 files due to include/exclude options
ninja: build stopped: subcommand failed.

Hence the change from //LCOV_EXCL_END to //LCOV_EXCL_STOP. However that does not fix the coverage CI error.

Comment thread test/hoctests/tests/test_many_ions.py Outdated
@azure-pipelines

Copy link
Copy Markdown

✔️ 74d6029 -> Azure artifacts URL

@nrnhines nrnhines mentioned this pull request Sep 27, 2024
@sonarqubecloud

Copy link
Copy Markdown

@azure-pipelines

Copy link
Copy Markdown

✔️ 1318a3f -> Azure artifacts URL

@codecov

codecov Bot commented Sep 30, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.89%. Comparing base (93c7d2c) to head (1318a3f).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3081      +/-   ##
==========================================
- Coverage   67.86%   66.89%   -0.97%     
==========================================
  Files         572      572              
  Lines      104210   106671    +2461     
==========================================
+ Hits        70718    71359     +641     
- Misses      33492    35312    +1820     

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

@1uc 1uc left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Given the complications, this seems sensible.

@nrnhines nrnhines merged commit 95cc05b into master Sep 30, 2024
@nrnhines nrnhines deleted the hines/ions branch September 30, 2024 21:54
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.

3 participants