search

lina-usc / pylossless

🧠 EEG Processing pipeline that annotates continuous data
https://pylossless.readthedocs.io/en/latest/
MIT License
20 stars 8 forks source link

Refactoring using XArray #22

Closed christian-oreilly closed 1 year ago

christian-oreilly commented 1 year ago

Refactoring of the code to use more comprehensively XArray. This avoids making a series of operations across axes specified with numerical indices, which is opaque and confusing. Now using name dimensions and coordinates.

Fixes #18

This PR also takes a big steps toward addressing #19 although some additional works need to be done to close that issue completely.

christian-oreilly commented 1 year ago

I am sure that there might be additional refactoring that will be done down the road, but the code of this branch is now working and fix #18. In the interest of avoiding divergence (and since their is no expectation of code stability yet), I recommend merging now. @scott-huberty are you OK with this?

scott-huberty commented 1 year ago

Yes. Merging now. Thanks @christian-oreilly !

christian-oreilly commented 1 year ago

Hey @scott-huberty Not a big problem, but just for the future: it is better for you to let me merge the PR I am leading. I had a few things I wanted to do before (e.g., last-minute small changes) and with the merging (squashing commits). I just needed you OK before moving along. I reverted the merge and I'll re-merge with these small changes.

scott-huberty commented 1 year ago

Oh sorry, I wasn't sure if I was the one that needed to merge!

christian-oreilly commented 1 year ago

Sure! No problem; just mentioning it for the future. I am currently running the version I have through a bunch of files... each one takes something like 1h to run (they are full recording; mostly long because of the ICA) so I am catching some bugs along the line and I want to include these within the PR before the merge. Should be done by the end of the weekend.