Meaningful Python types

[ramcdougal]

Context

All NEURON’s internal interpreter objects are instances of a global top-level type: HocObject. Before this PR they were considered direct instances, without any intermediate hierarchy.

For instance 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).

This PR

With this PR Hoc classes are now associated with actual Python types, created dynamically. Such change enables type instances to be properly recognized as such, respecting e.g. isinstance() predicates and subclassing. E.g.

>>> isinstance(hoc.Vector, type)
True
>>> v = h.Vector()
>>> isinstance(v, hoc.HocObject)
True
>>> isinstance(v, hoc.Vector)
True
>>> type(v) is hoc.Vector  # direct subclass
True
>>> isinstance(v, hoc.Deck)  # Not instance of other class
False

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

This happens with negligible degradation of performance thanks to hash mapping from NEURON symbol to Python object type.

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. It;s also an instance of type
• When using hclass, we can only have one HOC base class.

[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

[azure-pipelines]

✔️ c0eee1e -> Azure artifacts URL

[ramcdougal]

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?

[azure-pipelines]

✔️ 944bcbc -> Azure artifacts URL

[ramcdougal]

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.

[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.)

[Helveg]

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])

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.

[codecov-commenter]

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%)	⬆️
share/lib/python/neuron/tests/utils/checkresult.py	63.33% <66.66%> (ø)
share/lib/python/neuron/hclass3.py	87.50% <83.33%> (+0.83%)	⬆️
src/nrnpython/nrnpy_hoc.cpp	70.78% <92.59%> (+0.39%)	⬆️

... and 22 files with indirect coverage changes

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

[azure-pipelines]

✔️ 146643d -> Azure artifacts URL

[ramcdougal]

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

  nrn/test/pynrn/test_pyobj.py

  Line 150 in f242cc8

  assert E._hoc_type == neuron.h.List

• 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)

[azure-pipelines]

✔️ 97976fe -> Azure artifacts URL

[github-actions]

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

[github-actions]

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

[anilbey]

>>> 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.

[azure-pipelines]

✔️ 3ee944b -> Azure artifacts URL

[azure-pipelines]

✔️ f092099 -> Azure artifacts URL

[ferdonline]

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

outdated

[azure-pipelines]

✔️ 5b059f0 -> Azure artifacts URL

[ferdonline]

adding @nrnhines for a final review

[nrnhines]

Very nice!

[azure-pipelines]

✔️ f4be23d -> Azure artifacts URL

[bbpbuildbot]

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

Status and direct links:

• ✅ mac_m1_cmake_build: [cmake, ON, OFF, OFF, address]
• ✅ spack_setup
• ✅ build:nmodl
• ✅ build:neuron:nmodl:intel:legacy
• ✅ build:neuron:nmodl:intel:shared
• ✅ build:neuron:nmodl:nvhpc:acc:legacy
• ✅ build:neuron:nmodl:nvhpc:acc:shared
• ✅ build:neuron:nmodl:nvhpc:omp:legacy
• ✅ build:neuron:nmodl:nvhpc:omp
• ✅ test:neuron:nmodl:intel:legacy
• ✅ test:neuron:nmodl:intel:shared
• ✅ test:neuron:nmodl:nvhpc:acc:legacy
• ✅ test:neuron:nmodl:nvhpc:acc:shared
• ✅ test:neuron:nmodl:nvhpc:omp:legacy
• ✅ test:neuron:nmodl:nvhpc:omp