Update utils.py

[Sebastien-PILUSO]

Remove those line which involved region names that are different from the ones updated by the leaves_only step.
We could keep them as well, but with their updated names given by the leaves_only change ("_other").
See https://bbpteam.epfl.ch/project/issues/browse/BBPP134-837?focusedId=223892&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-223892

[mgeplf]

Hmm, interesting problem: this means we completely lose the ability to run this code on the "stock" AIBS atlas.
Let me give it some thought how to work around this.

Do you think there are other places where a similar workaround is necessary?

[drodarie]

These filters were very specific to the ccfbbp version (version from Csaba). They had very little effect on ccfv2 because they improved the fibers and none on ccfv3.
One solution could be to have a separated function to correct ccfbbp before running the pipeline but maybe not in atlas-densities.

[lecriste]

I think the best solution is to remove this hardcoded list of region names and pass it via a configuration file where one can just change "Cerebellum" with "Cerebellum: Other" or whatever new name that region got in the input hierarchy/annotation (or just drop it as in this PR).

[drodarie]

This would require a lot more work to deal with configurations that hold region names and make sure that the provided values makes sense. In that case we might as well refactor the whole pipeline to use my method instead of Csaba's for the cell and neuron densities estimation
.

[lecriste]

This would require a lot more work to deal with configurations that hold region names and make sure that the provided values makes sense.

It's already the case:

• markers
• measurements
• metadata
• mtypes

and many other configuration files I am not aware of.

[mgeplf]

@Sebastien-PILUSO > Remove those line which involved region names that are different from the ones updated by the leaves_only step.

We could keep them as well, but with their updated names given by the leaves_only change ("_other").

I'm still confused as to what the ramifications of this are: when I look at the underlying voxels that this removes (ie: Basic cell groups and regions, Cerebellum) they look like this:

Which seems like a non-trivial change. What results does this correct?

[lecriste]

Based on the discussion at today's SBO stand-up, one way forward would be to replace the five hardcoded region names in the fiber_tracts_ids with a configuration file containing the region names (and the corresponding with_descendants flag).
The default configuration file can look like

{
"fiber tracts": True,
"grooves": True,
"ventricular systems": True,
"Basic cell groups and regions": False,
"Cerebellum": False
} 

and the pipeline will customize it based on the input hierarchy/annotation.
@mgeplf, sounds good?

[mgeplf]

one way forward would be to replace the five hardcoded region names in the fiber_tracts_ids with a configuration file containing the region names (and the with_descendants flag).

That fixes the case of the fiber tracts, but there are other region groups (Cerebellum group, Isocortex group, Fiber tracts group, Purkinje layer, Molecular layer, Cerebellar cortex, Rest) that will also have to periodically be updated; the rules for how these are generated are more complex, so just fixing Fiber tracts group` will be papering over that one problem, but doesn't fix the general case with a config file.

In addition, it still requires the person running the pipeline to examine the code to know what config file does, so it's, imo brittle in the same way as embedding it in the code.

Finally, the fact that just removing the Basic cell groups and regions and Cerebellum may (!?) be a solution means that the config provided doesn't work - one would have to update both the config and the code to deal with that case...

[lecriste]

one way forward would be to replace the five hardcoded region names in the fiber_tracts_ids with a configuration file containing the region names (and the with_descendants flag).

That fixes the case of the fiber tracts, but there are other region groups (Cerebellum group, Isocortex group, Fiber tracts group, Purkinje layer, Molecular layer, Cerebellar cortex, Rest) that will also have to periodically be updated; the rules for how these are generated are more complex, so just fixing Fiber tracts group` will be papering over that one problem, but doesn't fix the general case with a config file.

Agree. We can find a fix for the general case later on as it requires much more work/time.

In addition, it still requires the person running the pipeline to examine the code to know what config file does, so it's, imo brittle in the same way as embedding it in the code.

The current config files in the repo are accompanied by a ReadMe, we can do the same for this one.

Finally, the fact that just removing the Basic cell groups and regions and Cerebellum may (!?) be a solution means that the config provided doesn't work - one would have to update both the config and the code to deal with that case...

Why? Once the hardcoded region names are replaced with a loop over the region names in the configuration file, one can update just the config file.

[drodarie]

Hi @mgeplf,

To respond to one of your comment earlier.

I'm still confused as to what the ramifications of this are

In very short, in older versions of the annotations, fibers and main regions were in separated files (ccfv2 / ccfbbp).
In the main regions, the space allocated to the fibers were annotated to Basic cell groups and regions. For some weird reasons, the fibers do not take the entire space allocated for them. Which means that for most of the voxels annotated to Basic cell groups and regions, they correspond to fibers. I say most of the voxels because there are also voxels at the frontier between two main regions (e.g. Cerebellum) that were not annotated to their closest leaf region. These voxels are gone in ccfv3.

So for ccfbbp and ccfv2, Csaba Erö and I decided that these voxels should be counted as fibers. Now with the renaming of the regions, obviously the filters did not work anymore...
For ccfv2, I thought that these voxels were less numerous so I suggested Sebastien to remove the two lines (thought this would have small impact). I may need to make some more analysis.

2 possible solutions:

• Extract the filter for fibers (with all the other region filters) and put them in configuration.
• Consider these voxels as non fibers or stop maintaining ccfv2/ccfbbp (remove the 2 lines)

[mgeplf]

@drodarie > [....] Which means that for most of the voxels annotated to Basic cell groups and regions, they correspond to fibers [...]

Thanks, that's very helpful. I will add commentary on the code to that effect.

[mgeplf]

Agree. We can find a fix for the general case later on as it requires much more work/time.

I'd prefer not to accumulate more technical debt like this. I will give a shot to writing a DSL this afternoon.

[mgeplf]

I'd prefer not to accumulate more technical debt like this. I will give a shot to writing a DSL this afternoon.

Ok, I have a draft of a DSL that covers the features that are currently in the group_ids code:
https://github.com/BlueBrain/atlas-densities/pull/55/files#diff-00c9a8a5df4192d39977d2572f5054e3ef8c5816a002a8108d51f9b673fc9e55

I still need to plumb it through the app, but most of the backend code is workikng.

[mgeplf]

I have everything in #55; I have tried running it with the default configs, and I seem to get the same output as before, so that's a promising sign.

[mgeplf]

Since #55 is merged, I think we can close this.