CompartmentSets

[cattabiani]

• fix make install -e work (again?) #3
• add CompartmentSets [Proposal]: Extending Compartment Report sonata-extension#5

API

CompartmentLocation

• init(name: str)
• init(gid: int64, section_idx: int64, offset: float)
• gid (property) -> int64
• section_idx (property) -> int64
• offset (property) -> float
• toJSON() -> str
• iter() -> tuple(gid, section_idx, offset)
• eq(other: CompartmentLocation) -> bool
• ne(other: CompartmentLocation) -> bool
• copy() -> CompartmentLocation
• deepcopy(memo: dict) -> CompartmentLocation
• repr() -> str
• str() -> str

CompartmentSet

• init(name: str)
• population (property) -> int
• size(selection=Selection()) -> int
• gids() -> List[int]
• filter(selection=Selection()) -> CompartmentSet
• filtered_iter(selection=Selection()) -> Iterator[Compartment]
• len() -> int
• getitem(index: int) -> Compartment
• iter() -> Iterator[Compartment]
• eq(other: CompartmentSet) -> bool
• ne(other: CompartmentSet) -> bool
• toJSON() -> str
• repr() -> str
• str() -> str

CompartmentSets

• init
• fromFile(path: str) -> CompartmentSets
• at(key: str) -> CompartmentSet
• contains(key: str) -> bool
• names() -> List[str]
• values() -> List[CompartmentSet]
• items() -> List[Tuple[str, CompartmentSet]]
• toJSON() -> str
• eq(other: CompartmentSets) -> bool
• ne(other: CompartmentSets) -> bool
• len() -> int
• getitem(key: str) -> CompartmentSet
• repr() -> str
• str() -> str

Additional changes

• add contains and its python api. Example 3 in Selection([0, 1, 2, 3]) == True
• ctests
• python tests I cannot find python tests for Selection at all. I will not add a full test suite just to test the pybinds of this function
• docs

[coveralls]

Pull Request Test Coverage Report for Build 15489697657

Details

• 293 of 324 (90.43%) changed or added relevant lines in 4 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.3%) to 92.59%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/config.cpp	8	10	80.0%
src/utils.h	17	21	80.95%
src/compartment_sets.cpp	260	285	91.23%

Totals
Change from base Build 15385836984:	-0.3%
Covered Lines:	2324
Relevant Lines:	2510

---

💛 - Coveralls

[cattabiani]

I still think this is necessary. I get this error if I try to run the command ./ci/python_test.sh:

  Successfully installed cmake-4.0.2 numpy-1.23.2 oldest-supported-numpy-2023.12.21 setuptools-80.8.0 wheel-0.45.1
  Installing build dependencies ... done
  Running command Getting requirements to build wheel
  Getting requirements to build wheel ... done
  Running command pip subprocess to install backend dependencies
  Using pip 25.1.1 from /Users/katta/OBI/libsonata/build/venv-python-test/lib/python3.11/site-packages/pip (python 3.11)
  Collecting setuptools_scm
    Obtaining dependency information for setuptools_scm from https://files.pythonhosted.org/packages/ab/ac/8f96ba9b4cfe3e4ea201f23f4f97165862395e9331a424ed325ae37024a8/setuptools_scm-8.3.1-py3-none-any.whl.metadata
    Using cached setuptools_scm-8.3.1-py3-none-any.whl.metadata (7.0 kB)
  Collecting packaging>=20 (from setuptools_scm)
    Obtaining dependency information for packaging>=20 from https://files.pythonhosted.org/packages/20/12/38679034af332785aac8774540895e234f4d07f7545804097de4b666afd8/packaging-25.0-py3-none-any.whl.metadata
    Using cached packaging-25.0-py3-none-any.whl.metadata (3.3 kB)
  Collecting setuptools (from setuptools_scm)
    Obtaining dependency information for setuptools from https://files.pythonhosted.org/packages/58/29/93c53c098d301132196c3238c312825324740851d77a8500a2462c0fd888/setuptools-80.8.0-py3-none-any.whl.metadata
    Using cached setuptools-80.8.0-py3-none-any.whl.metadata (6.6 kB)
  Using cached setuptools_scm-8.3.1-py3-none-any.whl (43 kB)
  Using cached packaging-25.0-py3-none-any.whl (66 kB)
  Using cached setuptools-80.8.0-py3-none-any.whl (1.2 MB)
  Installing collected packages: setuptools, packaging, setuptools_scm

  Successfully installed packaging-25.0 setuptools-80.8.0 setuptools_scm-8.3.1
  Installing backend dependencies ... done
  Running command Preparing metadata (pyproject.toml)
  WARNING setuptools_scm.pyproject_reading toml section missing 'pyproject.toml does not contain a tool.setuptools_scm section'
  Traceback (most recent call last):
    File "/private/var/folders/jd/wq32gh815w9c13zf3lkl7hqw0000gn/T/pip-build-env-c12fwv7i/normal/lib/python3.11/site-packages/setuptools_scm/_integration/pyproject_reading.py", line 36, in read_pyproject
      section = defn.get("tool", {})[tool_name]
                ~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^
  KeyError: 'setuptools_scm'
  /private/var/folders/jd/wq32gh815w9c13zf3lkl7hqw0000gn/T/pip-build-env-c12fwv7i/overlay/lib/python3.11/site-packages/setuptools/dist.py:759: SetuptoolsDeprecationWarning: License classifiers are deprecated.
  !!

          ********************************************************************************
          Please consider removing the following classifiers in favor of a SPDX license expression:

          License :: OSI Approved :: GNU Lesser General Public License v3 (LGPLv3)

          See https://packaging.python.org/en/latest/guides/writing-pyproject-toml/#license for details.
          ********************************************************************************

  !!

[mgeplf]

looks like the license identifier format has just been updated; so may as well fix that too

[cattabiani]

looks like the license identifier format has just been updated; so may as well fix that too

[cattabiani]

is this correct? @mgeplf license="LGPL-3.0-or-later",

[mgeplf]

is this correct? @mgeplf license="LGPL-3.0-or-later",

I think so?

nodesets are orthogonal from configuration, so it's best to leave them where they are.

[cattabiani]

nodesets are orthogonal from configuration, so it's best to leave them where they are.

and why leave them in the general h5 folder but circuit_config and simulation_config are not orthogonal? I was grouping by extension (json). In addition, to me nodesets are an optional addition of the configuration. I mean, ususally I find all these files together while the others h5 are in another folder

[mgeplf]

and why leave them in the general h5 folder but circuit_config and simulation_config are not orthogonal? I was grouping by extension (json)

It's a generic data folder, not one specific to h5. the config files happen to be json, but that doesn't mean all json files are config files. The config files correspond to the config.cpp file.

[cattabiani]

you can also see it as:

that folder contains all the h5 files from generate.py. It does not generate node_sets.json so it needs to go somewhere else the file

[mgeplf]

I don't think it's worth spending this much time over; it's been there since 2021, it doesn't fit under config, so can it just be left in its original place? We're not dealing w/ that many different data files here that a big reorganization is necessary.

[cattabiani]

@mgeplf I am implementing compartmentSets based on nodeSets. I understand that nodeSets are way more complicated and I need to shave away a lot of stuff. I wonder what should be the python API. At the moment I have this in bindings.cpp:

    py::class_<CompartmentSets>(m, "CompartmentSets", "CompartmentSets")
        .def(py::init<const std::string&>())
        .def_static("from_file", [](py::object path) { return CompartmentSets::fromFile(py::str(path)); })
        .def_property_readonly("names", &CompartmentSets::names, DOC_COMPARTMENTSETS(names))
        .def("toJSON", &CompartmentSets::toJSON, DOC_COMPARTMENTSETS(toJSON));

I was considering a get_compartment_set function. Do we need materialize? How should it work? What is your opinion?
If you want I can open an issue to track what should be the python API

[mgeplf]

materialize is to join a population with all the operations that are available in https://sonata-extension.readthedocs.io/en/latest/sonata_nodeset.html

So I don't think it's needed.

However, what will be needed, I think, is a way to easily extract compartment_set by name from the compartment_sets, and then pull subsets of the report out based on the contents, I think, or at least get the indices. Something like from, from compartment set xyz, give me the indices in the report for row 10.

[mgeplf]

CompartmentLocation has a unique_ptr to the implementation

Ah, right, we're running into the the pimpl problem.

I think some of this could be simplified if CompartmentLocation was a public Plain Old Data struct, what do you think?

[cattabiani]

@mgeplf somehow I cannot answer to this directly so I will answer here:

CompartmentLocation has a unique_ptr to the implementation
Ah, right, we're running into the the pimpl problem.
I think some of this could be simplified if CompartmentLocation was a public Plain Old Data struct, what do you think?

That would be a break of encapsulation (that is everywhere in the code): exposing the detail to python and the public api. However, I think Selection is the same. That would work, yes. I am just slightly afraid that the garbage collector may destroy stuff that is used somewhere else. I just have never faced the problem so I do not know how the garbage collector works in detail. Another solution is to use shared_ptr. However, with shared_ptr then I need a vector of shared_ptrs in CompartmentSet which breaks locality

[mgeplf]

That would be a break of encapsulation (that is everywhere in the code): exposing the detail to python and the public api.

Which is fine; the contents of Location don't need to be encapsulated, it just adds unnecessary overhead without gaining much.
Returning structs like that is pretty normal, IMO, and works well in practice.

I am just slightly afraid that the garbage collector may destroy stuff that is used somewhere else.

pybind will handle the copy/conversion of the Location structs when they are returned by ref, so that will be fine, afaik.

[cattabiani]

That would be a break of encapsulation (that is everywhere in the code): exposing the detail to python and the public api.

Which is fine; the contents of Location don't need to be encapsulated, it just adds unnecessary overhead without gaining much.
Returning structs like that is pretty normal, IMO, and works well in practice.

I am just slightly afraid that the garbage collector may destroy stuff that is used somewhere else.

pybind will handle the copy/conversion of the Location structs when they are returned by ref, so that will be fine, afaik.

ok, I can do that. The only questions/modifications remaining (IMHO) are this and the naming:

• keys -> names (ok) (already done)
• values -> getCompartmentSets (?)
• at -> getCompartmentSet (?)

do you have an opinion on this?

[cattabiani]

That would be a break of encapsulation (that is everywhere in the code): exposing the detail to python and the public api.

Which is fine; the contents of Location don't need to be encapsulated, it just adds unnecessary overhead without gaining much.
Returning structs like that is pretty normal, IMO, and works well in practice.

I am just slightly afraid that the garbage collector may destroy stuff that is used somewhere else.

pybind will handle the copy/conversion of the Location structs when they are returned by ref, so that will be fine, afaik.

done! The code is much simpler now. Sometimes breaking the rules is just simpler than enforcing paradigms

[mgeplf]

I think that's a lot nicer!
If you can remove the commented code, I think it's good to go.

[cattabiani]

oh I forgot. yes, done

[mgeplf]

Nice, thanks a lot!