Skip to content

Add support for Spike2 files#410

Merged
smoia merged 14 commits intophysiopy:masterfrom
htwangtw:read-spike
Apr 27, 2023
Merged

Add support for Spike2 files#410
smoia merged 14 commits intophysiopy:masterfrom
htwangtw:read-spike

Conversation

@htwangtw
Copy link
Copy Markdown
Contributor

@htwangtw htwangtw commented Jul 21, 2021 •
edited by smoia
Loading

Addresses #405.

The current implementation uses the official API, sonpy, from CED. sonpy is closed sourced.
In order to support sonpy, the minimal python version should be 3.7. Hence I will mark this as part of the major release.

The scanner volume trigger in the test file was stored as a "marker" channel. The sonpy API returns time points of markers, rather than a timeseries. The module I wrote has been tested locally. I cannot run the full test due to bandwidth and data limit. Will check if this breaks other tests.

Proposed Changes

  • Add phys2bids.io.load_smr

Change Type

  • bugfix (+0.0.1)
  • minor (+0.1.0)
  • major (+1.0.0)
  • refactoring (no version update)
  • test (no version update)
  • infrastructure (no version update)
  • documentation (no version update)
  • other

Checklist before review

  • I added everything I wanted to add to this PR.
  • [Code or tests only] I wrote/updated the necessary docstrings.
  • [Code or tests only] I ran and passed tests locally.
  • [Documentation only] I built the docs locally.
  • My contribution is harmonious with the rest of the code: I'm not introducing repetitions.
  • My code respects the adopted style, especially linting conventions.
  • The title of this PR is explanatory on its own, enough to be understood as part of a changelog.
  • I added or indicated the right labels.
  • I added information regarding the timeline of completion for this PR.
  • Please, comment on my PR while it's a draft and give me feedback on the development!

@htwangtw
Copy link
Copy Markdown
Contributor Author

htwangtw commented Jul 21, 2021 •
edited
Loading

Looks like conda doesn't support the latest version of sonpy - I am not familiar with Cirrus CI, if anyone can look into how to get pip to install certain libraries, that would be grand.

Found the reason. How sonpy uses release version number is unorthodox. It is an easy fix.

@htwangtw htwangtw mentioned this pull request Jul 21, 2021
Closed
smoia
smoia approved these changes Nov 22, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@smoia smoia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (let's wait for the tests)!

@codecov
Copy link
Copy Markdown

codecov bot commented Nov 22, 2021 •
edited
Loading

Codecov Report

Merging #410 (a54e85c) into master (d3a7443) will increase coverage by 0.76%.
The diff coverage is 100.00%.

❗ Current head a54e85c differs from pull request most recent head fea0e0d. Consider uploading reports for the commit fea0e0d to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #410      +/-   ##
==========================================
+ Coverage   94.25%   95.01%   +0.76%     
==========================================
  Files           8        8              
  Lines         974      903      -71     
==========================================
- Hits          918      858      -60     
+ Misses         56       45      -11
Impacted Files Coverage Δ
phys2bids/io.py 99.50% <100.00%> (+2.86%) ⬆️

... and 5 files with indirect coverage changes

@htwangtw
Copy link
Copy Markdown
Contributor Author

htwangtw commented Nov 22, 2021

hmmm all python 3.6 checks failed. Looks like a sonpy problem - they use the versioning numbers in a very special way (1.6.x -> python 3.6 support; 1.7.x -> python 3.7 etc)

@smoia
Copy link
Copy Markdown
Member

smoia commented Nov 22, 2021 •
edited
Loading

More than that, I believe that sonpy only supports python 3.7 and above (If I understood their page )
Python 3.6 reaches its EOL in a month, so I see two options here:

  • we add an exception to not run this test when running python 3.6, or
  • we can speed up things and end support for python 3.6 earlier on, removing its tests alltogether.

WDYT?

@htwangtw
Copy link
Copy Markdown
Contributor Author

htwangtw commented Nov 22, 2021

Ah good to know!
I would prefer

  1. explicitly exclude 3.6 for sonpy related functions
  2. end support for 3.6 in future iteration.

Dealing with deprecation and adding a new function in one PR might be too much

@smoia
Copy link
Copy Markdown
Member

smoia commented Nov 24, 2021

Agreed! Let's exclude 3.6 testing for sonpy.
Do you need help with it?

@htwangtw
Copy link
Copy Markdown
Contributor Author

htwangtw commented Nov 24, 2021

I will have a look and likely working on this next week

@smoia smoia mentioned this pull request Jan 7, 2022
Closed
@smoia smoia added the Minormod This PR generally closes an `Enhancement` issue. It increments the minor version (0.+1.0) label Apr 27, 2023
@smoia
Copy link
Copy Markdown
Member

smoia commented Apr 27, 2023

@htwangtw I tried to update this PR to merge it in.
At the moment, I still cannot figure out how to fix the installation of sonpy - mainly due to the fact that python 3.10 cannot install it.

As a temporary solution, I marked as xfail the sonpy test. I don't love this, but I don't know how to solve it at the moment.

If you agree with the changes, we merge!

htwangtw and others added 14 commits April 27, 2023 02:34
@htwangtw @smoia
NF/ADD CED spike2 file IO
15c082b 
The current implementation uses the official API, SONPY, from CED.
SONPY is closed sourced.

The scanner volumen trigger in the test file was stored as "marker"
channel. The SONPY API returns time points of markers, rather than a
timeserie. The module I wrote has been tested on client machine. Will
check if this breaks other tests.
@htwangtw @smoia
remove some print statement for sanity check
cd46284
@htwangtw @smoia
FIX installation options
5799932
@htwangtw @smoia
LINT flake8 don't like dictionary lined up like table
0003507
@htwangtw @smoia
FIX sonpy version using 1.7.5. They use the second digit for python v…
e69a858 
…ersion not minor release.
@smoia
Add python requirement in install
59008d9
@smoia
Try to fix install requirements
77f9b50
@smoia
Try again
15a2831
@smoia
Try again
08fd88a
@smoia
Try again
9a6e3ae
@smoia
Remove extra '
7628240
@smoia
Selective skip tests
13e4912
@smoia
Mark xfail
83d245c
@smoia
Run pre-commits
fea0e0d
@smoia smoia changed the title NF/ADD CED spike2 file IO Add support for Spike2 files Apr 27, 2023
@smoia smoia merged commit 6e96e9f into physiopy:master Apr 27, 2023
@smoia
Copy link
Copy Markdown
Member

smoia commented Apr 27, 2023

🚀 PR was released in 2.9.0 🚀

@smoia smoia added the released This issue/pull request has been released. label Apr 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@smoia smoia smoia approved these changes

Assignees

No one assigned

Labels

Minormod This PR generally closes an `Enhancement` issue. It increments the minor version (0.+1.0) released This issue/pull request has been released.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@htwangtw @smoia