BlueBrain / voxcell

Tools to work with voxel based brain atlases.
Apache License 2.0
5 stars 3 forks source link

Add RegionMap.from_dataframe() and RegionMap.as_dict() methods #32

Closed adrien-berchet closed 7 months ago

adrien-berchet commented 7 months ago

I need to create custom hierarchies and the nested dict structure is very boring to update so I would prefer updating the DataFrame representation and then be able to create a RegionMap object from it.

I just need a clarification: in the tests the root node ID is -1, which means that in the DataFrame representation it is its own parent since the root parent ID is always -1. I feel it weird so I changed the test to ensure that no node have a -1 ID. But I can change if you think that this situation actually makes sense, and I could consider that the first node with parent_id == -1 is the root node for example.

eleftherioszisis commented 7 months ago

Trying it with the full hierarchy:

from voxcell import RegionMap
r = RegionMap.load_json("mba_hierarchy.json")
df = r.as_dataframe()
r2 = RegionMap.from_dataframe(df)
should_be_None = r2.get(997, attr="parent_structure_id")
type(should_be_None)
>> float

Returns a float NaN which does not pass checks like X is None and is not consistent with the initial region map's None value.

adrien-berchet commented 7 months ago

Trying it with the full hierarchy:

from voxcell import RegionMap
r = RegionMap.load_json("mba_hierarchy.json")
df = r.as_dataframe()
r2 = RegionMap.from_dataframe(df)
should_be_None = r2.get(997, attr="parent_structure_id")
type(should_be_None)
>> float

Returns a float NaN which does not pass checks like X is None and is not consistent with the initial region map's None value.

Indeed. Is the conversion from nan to None specific to the parent_structure_id attr or should all the nan from any column be converted to None? Since the conversion to DF casts None to nan, I'm afraid it's hard to know when we should convert nan back to None, except if we know the list of columns for which the cast should be performed.

adrien-berchet commented 7 months ago

I'm afraid it's hard to know when we should convert nan back to None, except if we know the list of columns for which the cast should be performed.

Of maybe we can consider that we should convert nan to None for all columns with dtype=float?

codecov-commenter commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

:exclamation: No coverage uploaded for pull request base (main@42c4987). Click here to learn what that means.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #32 +/- ## ======================================= Coverage ? 94.68% ======================================= Files ? 13 Lines ? 1072 Branches ? 0 ======================================= Hits ? 1015 Misses ? 57 Partials ? 0 ``` | [Flag](https://app.codecov.io/gh/BlueBrain/voxcell/pull/32/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BlueBrain) | Coverage Δ | | |---|---|---| | [pytest](https://app.codecov.io/gh/BlueBrain/voxcell/pull/32/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BlueBrain) | `94.68% <100.00%> (?)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BlueBrain#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

eleftherioszisis commented 7 months ago

LGTM. Maybe @mgeplf would also like to take a look.