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

Test for PR 3055 #3081

Merged
merged 25 commits into from
Sep 30, 2024
Merged

Test for PR 3055 #3081

merged 25 commits into from
Sep 30, 2024

Conversation

nrnhines
Copy link
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.

Copy link

✔️ 7ccc143 -> Azure artifacts URL

@nrnhines
Copy link
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.

Copy link

✔️ 3f5c418 -> Azure artifacts URL

Copy link

✔️ da974c2 -> Azure artifacts URL

Copy link

✔️ 3f213b0 -> Azure artifacts URL

@nrnhines
Copy link
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.

Copy link

✔️ 23071ee -> Azure artifacts URL

Copy link

✔️ 6604aab -> Azure artifacts URL

Copy link

✔️ ebf26b1 -> Azure artifacts URL

@nrnhines
Copy link
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.

Copy link

✔️ 06f9ace -> Azure artifacts URL

@nrnhines
Copy link
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.

Copy link

✔️ 74d6029 -> Azure artifacts URL

@nrnhines nrnhines mentioned this pull request Sep 27, 2024
Copy link

sonarcloud bot commented Sep 30, 2024

Copy link

✔️ 1318a3f -> Azure artifacts URL

Copy link

codecov bot commented Sep 30, 2024

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.

Copy link
Collaborator

@1uc 1uc left a comment

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
37 checks passed
@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