Meaningful Python types by ramcdougal · Pull Request #1858 · neuronsimulator/nrn

ramcdougal · 2022-06-20T23:53:15Z

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 · 2022-06-21T00:34:35Z

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 · 2022-06-21T01:11:10Z

✔️ c0eee1e -> Azure artifacts URL

ramcdougal · 2022-06-21T01:35:50Z

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 · 2022-06-21T02:10:07Z

✔️ 944bcbc -> Azure artifacts URL

ramcdougal · 2022-06-21T09:33:27Z

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 · 2022-06-21T09:50:37Z

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 · 2022-06-21T10:06:19Z

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 · 2022-06-21T15:18:36Z

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 · 2022-06-21T17:07:25Z

✔️ 146643d -> Azure artifacts URL

ramcdougal · 2022-06-21T19:21:41Z

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 · 2023-09-26T02:53:57Z

✔️ 97976fe -> Azure artifacts URL

github-actions · 2023-09-26T12:26:41Z

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

github-actions · 2023-09-26T16:31:55Z

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

anilbey · 2023-09-27T15:19:22Z

>>> 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 · 2023-09-27T16:19:09Z

✔️ 3ee944b -> Azure artifacts URL

azure-pipelines · 2023-09-27T18:44:18Z

✔️ f092099 -> Azure artifacts URL

ferdonline · 2023-09-27T18:56:23Z

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

outdated

azure-pipelines · 2023-09-28T02:44:38Z

✔️ 5b059f0 -> Azure artifacts URL

ferdonline · 2023-09-28T14:02:11Z

adding @nrnhines for a final review

nrnhines

Very nice!

azure-pipelines · 2023-09-28T16:32:36Z

✔️ f4be23d -> Azure artifacts URL

bbpbuildbot · 2023-09-28T17:12:50Z

hoc.Vector distinct from hoc.HocObject

eb13ae2

ramcdougal added the python label Jun 21, 2022

ramcdougal assigned ramcdougal, ferdonline and Helveg Jun 21, 2022

This comment was marked as resolved.

Sign in to view

hoc.Vector is a subclass of hoc.HocObject

c0eee1e

generating specs dynamically allows for different names

944bcbc

check result checks type

381e6b1

h.Vector returns hoc.Vector; hclass not needed anymore

146643d

olupton added the hackron2022 label Jun 21, 2022

ramcdougal commented Jun 21, 2022

View reviewed changes