NeuroM icon indicating copy to clipboard operation
NeuroM copied to clipboard

Explicitly populate feature dictionaries with all available features.

Open eleftherioszisis opened this issue 2 years ago • 6 comments

This is a PoC of registering all features that will be available by features.get beforehand, instead of implicitly delegating a neurite, morphology or population or collections thereof, to the respective feature when get is called.

The _NEURITE_FEATURES, _MORPHOLOGY_FEATURES, _POPULATION_FEATURES dictionaries are updated following registration so that features from downstream categories (e.g. neurite features) are transformed and registered to upstream categories (e.g. morphology & population feature dictionaries).

If a feature is already registered in upstream categories, because it is defined in the respective module, it is not overwritten and the module definition is used instead.

This PoC allows to define reducible features, i.e. features that can be used by higher order objects by applying them to their components (e.g. a neurite feature can be applied to the neurites of a morphology or population). If a feature is not reducible it should be defined in each module (neurite.py, morphology.py, population.py) so the special logic is used instead.

The advantage of this change is that the available features become explicit and populate the respective feature dictionaries. In this context, features.get only needs to check what type of object is passed and get the respective function from the dictionaries without any special logic.

eleftherioszisis avatar Apr 05 '22 13:04 eleftherioszisis

See all available features as shown in the features.get help using this change: features.txt

eleftherioszisis avatar Apr 05 '22 13:04 eleftherioszisis

@adrien-berchet , @lidakanari , @arnaudon , please feel free to take a look if you have the time. I am not adding you as reviewers yet because it's still a PoC that needs to be discussed.

eleftherioszisis avatar Apr 05 '22 14:04 eleftherioszisis

It looks nice! I had a quick look and I am just wondering what happens in the current version for generators. Because with this PR, as far as I can see generators would not be caught by collections.abc.Sequence so it would fail, and I am wondering if it would be a breaking change.

adrien-berchet avatar Apr 05 '22 14:04 adrien-berchet

It looks nice! I had a quick look and I am just wondering what happens in the current version for generators. Because with this PR, as far as I can see generators would not be caught by collections.abc.Sequence so it would fail, and I am wondering if it would be a breaking change.

Thanks for checking it out! In the master's implementation, generators are not supported either: https://github.com/BlueBrain/NeuroM/blob/623cbd99ede31ae7d6434257593bbc59260c9aa2/neurom/features/init.py#L88-L91 Thus, I have not introduced a new behavior in this implementation.

Supporting generators is an interesting topic. Not sure if practical or if there are use cases that require them, but I believe with a bit of extra logic you could check the first element and return a chain of the first element with the rest of the generator to pass into the feature functions.

eleftherioszisis avatar Apr 05 '22 17:04 eleftherioszisis

Ok! I wondered about generators because of the Population objects since their __iter__ method returns a generator. So I think we could have the following use case: we iterate over a population using the generator, we change something in the morphology and yield this new morphology on which we want to compute the feature. In this case we would need to support generators. But it's quite specific.

adrien-berchet avatar Apr 06 '22 08:04 adrien-berchet

Ok! I wondered about generators because of the Population objects since their __iter__ method returns a generator. So I think we could have the following use case: we iterate over a population using the generator, we change something in the morphology and yield this new morphology on which we want to compute the feature. In this case we would need to support generators. But it's quite specific.

Certainly, we could address this in a separate issue.

eleftherioszisis avatar Apr 07 '22 08:04 eleftherioszisis