search

PIA-Group / BioSPPy

Biosignal Processing in Python
Other
573 stars 274 forks source link

Compatibility with python 3 #14

Closed DominiqueMakowski closed 7 years ago

DominiqueMakowski commented 7 years ago

Hey, I've come accross your package yesterday. And it seems really awesome. However, it is not compatible with python 3 😢

I'm pretty sure there are only few changes to make in order to fix it tho.

I did for the ecg function. However, while I applied it successfuly to my data (it got me a nice plot and all), I stumbled accross this weird (no offense 😆) return behavior. Even with your explanation in the docs, I couldn't unpack this return-thing. Why are you not packing the output into a dictionary, where each name is associated with a value that could be anything? It would be simpler and cleaner to get a single dictionary with the different parameters.

Also, I've implemented lazy loading of the signal functions. Since people are not using like 100 times the function ecg.ecg(), I think they would like to have the opportunity to keep the full name (i.e. biosppy.signals.ecg.ecg()) (altough they still can do the same as before).

Let me know if you plan to work and maintain this package,

Thanks, cheers,

DominiqueMakowski commented 7 years ago

Eeeew, I didn't see the other pull request which is about the same...

I'm sure it's much cleaner, don't mind me, I'll close this one 😃

capcarr commented 7 years ago

Hi @DominiqueMakowski Thanks for your interest in BioSPPy and your comments. As you already noticed, I'm reviewing #13 and will release a new version as soon as I'm able to figure out the best way to support both Python 2 and 3.

Regarding the return behavior, could you explain better what you aren't able to unpack, or what was the behavior you were expecting?

DominiqueMakowski commented 7 years ago

I succeeded unpacking them using .to_dict(), based on the version of #13. Anyway, thank you for your work it works superbly 😉