BlueBrain / morph-tool

https://morph-tool.readthedocs.org/
GNU Lesser General Public License v3.0
8 stars 7 forks source link

MorphLoader can now return mutable Morphology objects #61

Closed adrien-berchet closed 3 years ago

adrien-berchet commented 3 years ago

Context

The morphologies returned by MorphLoader are not mutable, so we have to call morph.as_mutable() to make it mutable before updating it. This PR allows to get mutable morphologies from MorphLoader.

Resolution

The MorphLoader constructor has a new parameter to choose whether the returned morphologies should be mutable or not.

adrien-berchet commented 3 years ago

I can't call for a reviewer on this repository. @asanin-epfl could you either review it or call for another reviewer plz?

asanin-epfl commented 3 years ago

Hi @adrien-berchet why do you want to incorporate this behaviour? How much it will lighten you code in terms of lines? For now, I see missing docstring updates, tests and general documentation misses the new flag. In general we try to minimize the base code of libraries.

adrien-berchet commented 3 years ago

In my case I just use it as a loader but I don't need the cache (I read the morphologies only once). I use it to define the loader in a quite global scope and then I can just get the morphology I need with 1 line. But since I need a mutable morphology, it takes 2 lines just to get the morphology and the 2nd line (morpho.as_mutable()) is not relevant in the context of loading a morphology, so I have to define another function to hide these 2 lines. So I think that this feature could be usefull for other users of the library. But of course, if you think it is not relevant it is ok. If you think it is I will update the PR to fix missing docstring.

asanin-epfl commented 3 years ago

In my case I just use it as a loader but I don't need the cache (I read the morphologies only once)

Do you use cache_size = 0 to avoid using cache?

if you think it is not relevant it is ok

I'm sorry but I can't find such change to be relevant.

adrien-berchet commented 3 years ago

ok then, np