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

formalization of an API #2357

Merged
merged 76 commits into from
May 31, 2024
Merged

formalization of an API #2357

merged 76 commits into from
May 31, 2024

Conversation

ramcdougal
Copy link
Member

@ramcdougal ramcdougal commented May 14, 2023

See src/nrniv/neuronapi.h for the list of API functions.

For examples of C++ code using this, see https://github.com/mcdougallab/neuron-c-api-demos/tree/main/for-proposed-formal-api

In particular, check out the hh_sim.cpp, vclamp.cpp, netcon.cpp, sections.cpp, and introspection.cpp files.

Many of the functions in the API are shallow and directly call a built-in, but this separation allows that to change in the future. Some others are more complicated. Importantly: the API completely hides details of the memory management; that is, users should be able to get and set all relevant properties using functions.

@ramcdougal ramcdougal linked an issue May 14, 2023 that may be closed by this pull request
@ramcdougal

This comment was marked as resolved.

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

@ramcdougal ramcdougal added this to the Release 9.0 Alpha milestone May 15, 2023
@ramcdougal
Copy link
Member Author

ramcdougal commented May 15, 2023

I'll have to figure out why this apparently doesn't build on anything other than my machine, but a mostly complete draft of a formal interface is in src/nrniv/nrnapi.h.

I would like suggestions for how to do iterators with C (as opposed to C++) because it feels like this is almost but not reasonable:

    sli = nrn_new_sectionlist_iterator(nrn_get_sectionlist_data(seclist));
    for (;!nrn_sectionlist_iterator_done(sli);) { 
        auto sec=nrn_sectionlist_iterator_next(sli);
        cout << "    " << nrn_secname(sec) << endl;
    }
    nrn_free_sectionlist_iterator(sli);

(That's from the sections.cpp test)

Other priorities to consider: how consistent is the naming convention?

@bbpbuildbot

This comment has been minimized.

src/nrniv/nrnapi.h Outdated Show resolved Hide resolved
src/nrniv/nrnapi.h Outdated Show resolved Hide resolved
src/nrniv/nrnapi.h Outdated Show resolved Hide resolved
src/nrniv/nrnapi.h Outdated Show resolved Hide resolved
src/nrniv/nrnapi.h Outdated Show resolved Hide resolved
src/nrniv/nrnapi.h Outdated Show resolved Hide resolved
src/nrniv/nrnapi.h Outdated Show resolved Hide resolved
src/nrniv/nrnapi.h Outdated Show resolved Hide resolved
src/nrniv/nrnapi.h Outdated Show resolved Hide resolved
src/nrniv/nrnapi.h Outdated Show resolved Hide resolved
On some systems (most? but not the one I used for everything else),
some files tried to include the wrong nrnapi.h during the
"make install" phase.
@codecov
Copy link

codecov bot commented May 15, 2023

Codecov Report

Attention: Patch coverage is 68.45361% with 153 lines in your changes are missing coverage. Please review.

Project coverage is 67.20%. Comparing base (30b42a1) to head (9e53b77).

Files Patch % Lines
src/nrniv/neuronapi.cpp 45.96% 134 Missing ⚠️
src/nrniv/ocjump.cpp 57.14% 6 Missing ⚠️
src/nrnoc/memblist.cpp 82.75% 5 Missing ⚠️
test/api/test_common.h 58.33% 5 Missing ⚠️
test/api/hh_sim.cpp 97.36% 1 Missing ⚠️
test/api/netcon.cpp 97.72% 1 Missing ⚠️
test/api/vclamp.cpp 96.96% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           master    #2357    +/-   ##
========================================
  Coverage   67.20%   67.20%            
========================================
  Files         563      569     +6     
  Lines      104246   104697   +451     
========================================
+ Hits        70057    70365   +308     
- Misses      34189    34332   +143     

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

@azure-pipelines
Copy link

✔️ e6afbb0 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

Copy link

✔️ be7261e -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

Copy link

✔️ aed536f -> Azure artifacts URL

  - after RANDOM construct (#2627) was merged,
    netstim.mod uses Random123 by default
  - before this, netstim was using scoprand by default
    and reference results were from the same
  - now, explicitly use Random123 in the test and update
    reference results
@pramodk
Copy link
Member

pramodk commented Mar 19, 2024

I did a merge with the master but test/api/netcon.cpp was failing. I updated results for Random123 and explained the reasoning in 030985e.

@ramcdougal: could you take a look at the last few comments and we can close this? 😃

Copy link

✔️ 030985e -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

Copy link

✔️ 74d66db -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

Copy link

Quality Gate Passed Quality Gate passed

Issues
46 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link

✔️ 50d7e68 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

@nrnhines
Copy link
Member

@ramcdougal I did not have an issue on my Apple M1 (Clang 15.0.0) with import neuron prior to the last merge from master. And I still don't have the issue after that merge. I guess we'll see if CI now passes.

Copy link
Member

@nrnhines nrnhines left a comment

Choose a reason for hiding this comment

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

@ramcdougal If this is working for your Matlab project, it seems worth merging to master when it passes CI. Any improvements can be dealt with in future PRs.

Copy link

✔️ 7e84d4b -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

Copy link

Quality Gate Passed Quality Gate passed

Issues
46 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link

✔️ 9e53b77 -> Azure artifacts URL

@ramcdougal ramcdougal merged commit 46dc539 into master May 31, 2024
37 checks passed
@ramcdougal ramcdougal deleted the api branch May 31, 2024 19:24
@ferdonline ferdonline restored the api branch July 4, 2024 13:01
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.

Defining a C API
6 participants