FZJ-INM1-BDA / siibra-python

Software interfaces for interacting with brain atlases - Python client
Apache License 2.0
46 stars 8 forks source link

refactor: use attributes #594

Open xgui3783 opened 1 month ago

xgui3783 commented 1 month ago

supercedes https://github.com/FZJ-INM1-BDA/siibra-python/pull/553

EDIT: The tests are turned off, except for the ones in examples_new to see if there is regression. The remaining test migration will be made gradually. (b70abe9f03533c684207462d5407dee96bed459d and 8a47f0feff6d6bd760ddce3e1ba0df1246c0955a)

xgui3783 commented 1 month ago

attn @AhmetNSimsek

per our discussion, here is the WIP

left to do:

refactor todo list

AhmetNSimsek commented 2 weeks ago

region.find is not deterministic and rather slow. I am noting down to investigate later.

AhmetNSimsek commented 2 weeks ago

@xgui3783, I think we should remove the dataclass decorators from classes that inherit other a class that has a dataclass decorator. Because it creates unexpected behaviors such as some methods are overridden and very difficult to debug as it is an unexpected behavior. Regarding being explicit: if a class A inherits a class B with dataclass decorators, the developer would know that A is also a dataclass. So, by repeating the 2 lines required to decorate class A as a dataclass, we break DRY, we face unexpected behavior, and we don't gain anything besides being explicit which is not really necessary in this context.

This changes if the class has extra members. Then, it is needed to use the decorator as the __init__ needs to be overridden. I suggest we remove all we can and leave only the necessary implementations. What do you think?