mne-python icon indicating copy to clipboard operation
mne-python copied to clipboard

CURRY Data Format Reader only works in specific cases

Open CurryKaiser opened this issue 1 year ago • 12 comments

Description of the problem

Dear MNE Team!

I am from the Curry Dev team and our users often ask about your Curry reader. We believe that it isn't up to date / complete and would like to offer some help (if wanted).

Attached you can find a python script (.py but hiding as .txt) that we use (and maintain) to load Curry data into python. Maybe the resulting data structure can be used by you to transform it according to MNE conventions.

curryreader.txt README.txt requirements.txt

If you need any test data, we would be happy to provide! We know that this might not be of highest importance, so we would understand if it isn't something that can be done at the moment.

Best Regards, Fabian Kaiser Compumedics - Curry Dev Team

Steps to reproduce

N/A

Link to data

No response

Expected results

N/A

Actual results

N/A

Additional information

N/A

CurryKaiser avatar Sep 16 '24 16:09 CurryKaiser

Hello! 👋 Thanks for opening your first issue here! ❤️ We will try to get back to you soon. 🚴

welcome[bot] avatar Sep 16 '24 16:09 welcome[bot]

hi @CurryKaiser, yes can you share a super tiny file eg 5MB so we can use it for testing when integrating this code? thanks

agramfort avatar Sep 19 '24 06:09 agramfort

Yes of course! Actually, I have prepared a few files for you: SampleFiles.zip Let me know how it goes. Hope it isn't too much of a hassle and thank you for all your efforts!

CurryKaiser avatar Sep 20 '24 10:09 CurryKaiser

thanks. Can you clarify what are files .cdt.dpo vs .cdt.dpa? I only see issues with the file MEG_A + MEG_B + Oth.cdt as it has a .dpo file (no .dpa)? Is it a different curry version?

agramfort avatar Sep 21 '24 14:09 agramfort

of course. .dpo and .dpa files are parameter and sensor geometry files. .dpo files are written at the time of aquisition and .dpa files are written when the .cdt files are opened for analysis. .dpa takes precedence over .dpo if it exists and this is important to keep the original data and parameters.

Maybe it could be worth considering to base the MNE Curry reader on our Python reader i.e. calling it first then just converting the output into MNE file format. This way whenever we add something to our file format, your reader would be up to date as well, as we maintain the Python reader. Though it might be a little early to tell if that is less work.

A collegue of mine has briefly skimmed the code of your Curry reader in the past and noticed some bugs and maybe some things missing but he never got around to reporting it and quite some time has passed. It would be a relief though if it is just the .dpo file that couldn't be read (as it is very similar to a .dpa file).

CurryKaiser avatar Sep 22 '24 09:09 CurryKaiser

yes we could base our reader on your code. For packaging reasons, we would need to have your code in a released package in pypi and conda (although we can help with that). Would it be possible for you to do this? Also when you find bugs in our code please open an issue in our tracker so action can be taken asap. thanks

Message ID: @.***>

agramfort avatar Sep 22 '24 16:09 agramfort

It might be possible. But I wanna hold off on commenting any further until our Python Reader expert is back which will be in 2 weeks. Yeah sorry about the unreported bug, it has been a long while ago and I am sure my collegue was swamped with work at that time.

CurryKaiser avatar Sep 24 '24 14:09 CurryKaiser

Apologies for the delay.

These were the issues so far that our expert did spot:

  1. Missing "cdt.dpo" dpo

  2. Landmark tokens: landmarks

  3. Issue with dev_head_transform: dev_head_transform

I will get back to you with pypi and conda packages. I think that should prevent any headaches in the future.

CurryKaiser avatar Oct 17 '24 11:10 CurryKaiser

At this point, it is unsure whether we can release the packages in the way that you suggest. I will keep you up to date when a decision is made.

CurryKaiser avatar Oct 17 '24 11:10 CurryKaiser

Unfortunately, we cannot. But our latest reader is always on github: https://github.com/neuroscan/curry-python-reader Thank you for your time!

CurryKaiser avatar Nov 13 '24 11:11 CurryKaiser

@CurryKaiser you probably saw that one of our contributors (@dominikwelke) has started integrating your https://github.com/neuroscan/curry-python-reader into MNE-Python over in https://github.com/mne-tools/mne-python/pull/13176. After some further discussion over there, we seem to be coalescing on a plan to:

  1. fork your reader repository
  2. add what is necessary to make it an installable Python package
  3. package and distribute it via PyPI and conda-forge
  4. depend on that package in MNE-Python, use it to do the low-level Curry reading operations, and write some wrapper code around it to create MNE-Python objects.

in your comment above, you indicated you aren't willing to package it yourselves (with or without our help). Since the code is BSD-licensed I guess we would be within our rights to do steps 1-4 regardless, but I wanted to keep you in the loop before we take those steps, in case anything has changed on your end. Would you still prefer that the repo neuroscan/curry-python-reader be left up to your people, and not be the source of an installable curry reader package? If so, we'll move forward with packaging/distributing/etc as described above.

drammock avatar Mar 31 '25 19:03 drammock

Oh sure that´s fine, as I indicated on the other thread, sorry for the late reply. I guess we can close this issue since the other one can take over. Yes we would like to keep our repo and you can be the source of an installable curry reader package. Thanks again :)

CurryKaiser avatar Apr 01 '25 12:04 CurryKaiser