Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for Spike2 files #410

Merged
merged 14 commits into from
Apr 27, 2023
Merged

Add support for Spike2 files #410

merged 14 commits into from
Apr 27, 2023

Conversation

htwangtw
Copy link
Contributor

@htwangtw htwangtw commented Jul 21, 2021

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
Contributor Author

htwangtw commented Jul 21, 2021

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.

Copy link
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

codecov bot commented Nov 22, 2021

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
Contributor Author

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
Member

smoia commented Nov 22, 2021

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
Contributor Author

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
Member

smoia commented Nov 24, 2021

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

@htwangtw
Copy link
Contributor Author

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

@smoia smoia mentioned this pull request Jan 7, 2022
@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
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
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.
@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
7 checks passed
@smoia
Copy link
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
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
Development

Successfully merging this pull request may close these issues.

None yet

2 participants