search

EIT-ALIVE / eitprocessing

Software for electrical impedance tomography data loading, visualization and processing
https://eit-alive.github.io/eitprocessing/
Apache License 2.0
5 stars 1 forks source link

Move responsibilities to `DataContainer` class #278

Open DaniBodor opened 4 months ago

DaniBodor commented 4 months ago

See comments/discussion in #275

DaniBodor commented 4 months ago

Ideally, the superclass would take define some common fields (attributes). However, any field (attribute) defined in the superclass would be first in de init function. label and name have default values in EITData, which would not work with non-default fields like path. Also, path would no longer be the first argument, potentially breaking existing code. To move fields toDataContainer, they would have to be the first arguments, or we would have to switch to keyword-only arguments.

I agree with this, and I think this should be implemented. I also think that the first fields should be identical for each container, and then the specific arguments per container should follow after that. Also, I think that nomenclature should be aligned more. For example, vendor and pixel_impedance in EITData in my mind is the equivalent of category and values (respectively) in the other containers.

Regarding breaking existing code, this is an argument that you have made a few times now (e.g. in issue #262). I don't think we have a large enough user base at this point to make that a prohibitive reason not to change things. We are very much still in the developmental phase of this project, and it's more important to "get things right", even if that means breaking changes, than ensuring backwards compatibility.

I think these kinds of changes can be implemented in a separate PR though, so that this one can stay light, as you suggest.

NOTE: comment copied from here

DaniBodor commented 4 months ago

Also, consider moving the methods from the current mixin-classes directly to DataContainer rather than inheriting them one by one.

psomhorst commented 4 months ago

Also, consider moving the methods from the current mixin-classes directly to DataContainer rather than inheriting them one by one.

This would bunch a whole load of unrelated methods together, making it very difficult to understand the separate functionalities and responsibilities. I would vote against that, unless we can reduce the total codebase to a few hundred lines.

Also, I'd rather spend time working on new functionality rather than refactoring working stuff.