nrn Meaningful Python types

We want h.Vector() to return an object with a different data type than h.Deck(). In current NEURON, they both are of type hoc.HocObject. Likewise for all other built-in NEURON classes. The types should display meaningful names when printed (e.g. hoc.Vector or maybe nrn.Vector, depending on where they are defined).

Additionally, each of the built-in class types should be a subclass of hoc.HocObject, for backwards compatibility.

This should all happen with neglible degradation of performance. Instead of repeated ifs or a switch, we may want to use a hash array for O(1) mapping from NEURON symbol to Python object type.

Ideally we should be able to subclass e.g. h.Vector without resorting to the hclass trick.

Some questions:

Should we do anything for classes defined in HOC? Does the answer change depending on if they're part of the NEURON library? (e.g. h.Import3D_GUI)
Is the hoc module the right place for the subtypes? Should they go in nrn instead? I would prefer to think of an h.Vector as a NEURON Vector than as a HOC Vector.

Additional considerations:

All returned NEURON objects are created by nrnpy_ho2po, but newly created NEURON objects are not (possibly due to nrnpy_ho2po increasing the NEURON reference count, but that ot being needed at object creation).
What should the type of h.Vector itself be? After the first three commits below, it's a hoc.HocObject still, but this is problematic for inheritance.
When using hclass, we can only have one HOC base class... is that a fundamental limitation of the interpreter that we need to enforce, or no?

Jun 20 '22 23:06 ramcdougal

The above commit makes the following work:

from neuron import h, hoc

v = h.Vector([1,2,3])

assert(type(v) == hoc.Vector)
assert(not (type(v) == hoc.HocObject))
assert(not (type(h.Deck()) == hoc.Vector))
assert(type(h.Deck()) == hoc.HocObject)

However:

While v is correctly identified as a hoc.Vector, it prints out incorrectly:

>>> type(v)
<class 'hoc.HocObject'>

Furthermore, the hoc.Vector type is not currently a subclass of hoc.HocObject:

>>> isinstance(v, hoc.Vector)
True
>>> isinstance(v, hoc.HocObject)
False

Most problematically, the new hoc.Vector class has broken equality comparisons (I'm guessing there's a check for whether or not we're comparing to a hoc.HocObject):

>>> v == v
False

Jun 21 '22 00:06 ramcdougal

✔️ c0eee1eb45cd1f79ee6a28b92bb255160361635c -> Azure artifacts URL

Jun 21 '22 01:06 azure-pipelines[bot]

With the above commit, the different types print out differently. In particular, the following now works:

from neuron import h, hoc

v = h.Vector([1,2,3])
w = h.Vector([1,5])
assert(type(v) == hoc.Vector)
assert(not (type(v) == hoc.HocObject))
assert(not (type(h.Deck()) == hoc.Vector))
assert(type(h.Deck()) == hoc.HocObject)
assert(isinstance(v, hoc.HocObject))
assert(isinstance(v, hoc.Vector))
assert(not isinstance(h.Deck(), hoc.Vector))
assert(str(type(v)) == "<class 'hoc.Vector'>")
assert(str(type(h.Deck())) == "<class 'hoc.HocObject'>")
assert(v == v)
assert(v != w)

The only remaining issue for the "get one working" stage that I'm aware of is allowing inhertiance without hclass.

... that and test/cover/test_netcvode.py fails for some reason. @nrnhines any idea?

Jun 21 '22 01:06 ramcdougal

✔️ 944bcbc0b0d57cc552df406b0a2b647109553f9b -> Azure artifacts URL

Jun 21 '22 02:06 azure-pipelines[bot]

For generalizing to all built-in classes, note that these are registered with the underlying NEURON stack machine by a call to class2oc as in here.

I'm not sure if HOC template definitions also call that function.

Jun 21 '22 09:06 ramcdougal

Maybe the inheritance issue can be addressed by having h.Vector return the hoc.Vector type object... and by making that distinct enough from hoc.HocObject that NEURON knows that passing something to hoc.Vector should create a NEURON Vector object... at the very least, it would avoid this unpleasantness:

>>> hoc.Vector([1,2,3])
<TopLevelHocInterpreter>

(By contrast right now, h.Vector returns a hoc.HocObject representing the template as opposed to the top-level-interpreter... the different types of HocObjects are distinguished via a type_ and whatever data they need.)

Jun 21 '22 09:06 ramcdougal

Maybe the inheritance issue can be addressed by having h.Vector return the hoc.Vector type object... and by making that distinct enough from hoc.HocObject that NEURON knows that passing something to hoc.Vector should create a NEURON Vector object... at the very least, it would avoid this unpleasantness:

hoc.Vector([1,2,3]) <TopLevelHocInterpreter>

I think the HocTopLevelInterpreter comes from how hocobj_new works: It sets self->type_ = PyHoc::HocTopLevelInterpreter;, and only if hocbase is passed to hocobj_new keyword args, will it overwrite it with a more appropriate type_ (usually PyHoc::HocObject). Since hoc.Vector is a subclass of hoc.HocObject, that new is called, but no hocbase kwarg is passed so it is PyHoc::HocTopLevelInterpreter. Could you confirm this by using:

hoc.Vector([1,2,3], hocbase=h.Vector)

I don't see why it is strictly necessary to keep the hocobj_new the way it is, since it was rigged exactly because there were no true (sub)types. Now there will be, and we can probably set the type_ subtype information in a much neater way!

Or, what I would do if I were cpp proficient, is go the hardcore way and refactor every occurence of type_ to ob_type and drop the symbols in favor of PyTypeObject checking. But that'll take a few Rocky montages for me to get there.

Jun 21 '22 10:06 Helveg

Codecov Report

Merging #1858 (f4be23d) into master (53c9392) will increase coverage by 0.01%. Report is 2 commits behind head on master. The diff coverage is 90.66%.

@@            Coverage Diff             @@
##           master    #1858      +/-   ##
==========================================
+ Coverage   61.48%   61.49%   +0.01%     
==========================================
  Files         623      623              
  Lines      119164   119200      +36     
==========================================
+ Hits        73265    73303      +38     
+ Misses      45899    45897       -2

Files	Coverage Δ
src/nrnoc/init.cpp	`89.82% <100.00%> (ø)`
src/oc/hoc_oop.cpp	`76.10% <100.00%> (+0.07%)`	:arrow_up:
share/lib/python/neuron/tests/utils/checkresult.py	`63.33% <66.66%> (ø)`
share/lib/python/neuron/hclass3.py	`87.50% <83.33%> (+0.83%)`	:arrow_up:
src/nrnpython/nrnpy_hoc.cpp	`70.78% <92.59%> (+0.39%)`	:arrow_up:

... and 22 files with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

Jun 21 '22 15:06 codecov-commenter

✔️ 146643d4f32fd6a7324160336ca1280721a44ce5 -> Azure artifacts URL

Jun 21 '22 17:06 azure-pipelines[bot]

I think everything that was identified early on works now.

You can inherit directly from e.g. h.Vector and an instance of h.Vector has type hoc.Vector.

Seven things before this is done-done:

clean up code (e.g. rename variable location, fix C++ style)
move things from hoc module to nrn module
make sure hclass still works with e.g. Vector to avoid breaking legacy code
add corresponding tests
fix the tests, e.g. the explicit type comparisons in https://github.com/neuronsimulator/nrn/blob/f242cc8d4ddefb74f71f7f613a4b7b4043c2b641/test/pynrn/test_pyobj.py#L150
the dir of subclasses should contain the dir of their parents. This is not blocking as the hclass solution doesn't provide this either.
find a way to block multiple inheritance from HOC objects, because our solution doesn't support that (nor did the hclass solution)

Jun 21 '22 19:06 ramcdougal

@ramcdougal @Helveg I tried fixing it by checking the places where we could corrupt memory, but unfortunately the problem is still there. There were a couple of places it seemed we needed Py_INCREF and some others the way around, but I'm very new to c API, so maybe an attentive review of these places could help reveal the bug. I'v started to setup the env to debug but takes time...

Jul 12 '22 08:07 ferdonline

I wonder if a review from @nrnhines would reveal the hiding bug

Jul 19 '22 08:07 ferdonline

@ramcdougal @Helveg We are thinking about reviving this for the next Hackathon. IIRC we were not far from feature complete, except for the CI crash. I can start by rebasing carefully to see if I spot anything fishy. Anything else to take into consideration? It would be nice if we could get this in :)

Aug 30 '23 09:08 ferdonline

That would be nice! I believe we were a single double free error away from a complete feature.

Sep 06 '23 15:09 Helveg

Agree. I've been meaning to add this to the hackathon list. I hope this is a quick fix.

Sep 06 '23 16:09 ramcdougal

@Helveg: Just FYI: Fernando, Robert, and several other team members will be meeting next week to wrap up long-pending PRs like this (Hackathon @ Yale from September 25th to 28th).

In case you have time and would like to just drop by via Zoom, please feel free to do so. I'll share the invite.

Sep 21 '23 23:09 pramodk

✔️ f0a068c4ed5621cc00fdc66ba6d921b6acdd570d -> Azure artifacts URL

Sep 25 '23 19:09 azure-pipelines[bot]

✔️ bb8435b031a135a5eefe16934e2fb9ead97f97f5 -> Azure artifacts URL

Sep 25 '23 20:09 azure-pipelines[bot]

Logfiles from GitLab pipeline #155660 (:no_entry:) have been uploaded here!

Status and direct links:

Sep 25 '23 21:09 bbpbuildbot

✔️ 97976fee39459044925ff92441c289272323c21a -> Azure artifacts URL

Sep 26 '23 02:09 azure-pipelines[bot]

Logfiles from GitLab pipeline #155689 (:white_check_mark:) have been uploaded here!

Status and direct links:

Sep 26 '23 03:09 bbpbuildbot

NEURON ModelDB CI: launching for 97976fee39459044925ff92441c289272323c21a via its drop url

Sep 26 '23 12:09 github-actions[bot]

NEURON ModelDB CI: 97976fee39459044925ff92441c289272323c21a -> download reports from here

Sep 26 '23 16:09 github-actions[bot]

Logfiles from GitLab pipeline #156456 (:white_check_mark:) have been uploaded here!

Status and direct links:

Sep 27 '23 14:09 bbpbuildbot

>>> type(neuron.h.ParallelContext())
<class 'hoc.ParallelContext'>
>>> type(neuron.h.Random())
<class 'hoc.Random'>
>>> type(neuron.h.Vector())
<class 'hoc.Vector'>

Wow great. Thanks everyone who's involved. I'm looking forward to using the types.

Sep 27 '23 15:09 anilbey

✔️ 3ee944ba671bf04b1117c843ecb0890d8d6a785f -> Azure artifacts URL

Sep 27 '23 16:09 azure-pipelines[bot]

Logfiles from GitLab pipeline #156598 (:no_entry:) have been uploaded here!

Status and direct links:

Sep 27 '23 17:09 bbpbuildbot

✔️ f0920992e611f0cd64ea0c5f3ff98b99bed6bafb -> Azure artifacts URL

Sep 27 '23 18:09 azure-pipelines[bot]

Second pipeline with BlueConfigs tests just passed https://bbpgitlab.epfl.ch/hpc/sim/blueconfigs/-/jobs/929615

Sep 27 '23 18:09 ferdonline

✔️ 5b059f089918ecdaee244365932c32adae7aaf65 -> Azure artifacts URL

Sep 28 '23 02:09 azure-pipelines[bot]

nrn nrn copied to clipboard

Meaningful Python types

Codecov Report

nrn
nrn copied to clipboard