Switch from attribute 'name' to 'acronym' #42
Conversation
| @@ -456,7 +456,7 @@ def get_aibs_region_names(region_map: "RegionMap") -> Set[str]: | |||
|
|
|||
| def get_hierarchy_info( | |||
| region_map: "RegionMap", | |||
| root: str = "Basic cell groups and regions", | |||
There was a problem hiding this comment.
This isn't the root name, was Basic cell groups and regions also changed?
There was a problem hiding this comment.
No, I just replaced it with its acronym as that's what the function is expecting now.
There was a problem hiding this comment.
But based on the PR description, the change is required (?) because the name of the root region changed from "root" to "Whole Mouse Brain".; so I'm confused as to why this change is required?
There was a problem hiding this comment.
Since the new code is looking for the acronym attribute of a region rather than for the name, the "root" argument of this function must be the acronym now.
There was a problem hiding this comment.
I meant, why do we need to change to acronym from "name", when the only places that are changing in this PR weren't affected by the change of name?
There was a problem hiding this comment.
I'd also like to change this, but this code can be used as a library, so changing fundamental API calls like this feels dangerous, so the bar for making changes is high. Are you calling this somewhere where you got caught by the change in names?
There was a problem hiding this comment.
Yes, I ran one step of the main ReadMe with the new version of the hierarchy and got an empty result because there is no longer a region with name "root".
The usage of acronym already exists.
There was a problem hiding this comment.
one step of the main ReadMe
Which step? The examples in the README are meant to be run by external uses of the code and they don't have access to the internal hierarchy.
To maintain backwards compat, we could add an attr argument, which defaults to name, but can be overridden to be acronym, what do you think?
There was a problem hiding this comment.
Which step? The examples in the README are meant to be run by external uses of the code and they don't have access to the internal hierarchy.
I ran this step with the new hierarchy as input.
To maintain backwards compat, we could add an
attrargument, which defaults toname, but can be overridden to beacronym, what do you think?
That's an option. The overwriting will happen for this call of get_hierarchy_info.
@mgeplf, shall I proceed this way?
There was a problem hiding this comment.
I have created #45 which allows one to choose the name of the region that is being fit. Thus, one should be able to use alternate hierarchies.
As of https://bbpteam.epfl.ch/project/issues/browse/BBPP134-36, the
nameof the root region changed from "root" to "Whole Mouse Brain".acronymis already used here.