search

BEL-Public / mffpy

Reader and Writer for Philips' MFF file format.
Other
10 stars 7 forks source link

Refactor association of epochs and categories #34

Closed jusjusjus closed 4 years ago

ephathaway commented 4 years ago

@jusjusjus the reason I included the association of category names within Reader.epochs is because I can't think of a situation where you wouldn't want the category names if they are available. From a user perspective, I think it makes more sense to be able to run Reader.epochs and have the category names for each epoch than to have to run Reader.associate_categories_with_epochs() first.

Maybe a private function Reader._associate_categories_with_epochs that gets called when you do Reader.epochs?

jusjusjus commented 4 years ago

From what you say, I think it's ok to associate Categories with epochs. I think this logic should be handled in class Epochs. Also, we should see if the exact times match between the two xml times to make sure no stray file is affecting the analysis.

ephathaway commented 4 years ago

I think that could work. So maybe a Epochs.add_category_names() which gets the category names if they are available. Is still think Reader.epochs should return the epochs with the category names filled in automatically.

jusjusjus commented 4 years ago

@ephathaway according to our conversation I refactored category association into class Epochs. If you consider merging this into your branch, please make sure that returning Epochs instead of list[epochs] doesn't cause any problems downstream. Please also consider adding some more documentation and testing, as well as code clean up if you like.