search

kylemath / mne-python

MNE : Magnetoencephalography (MEG) and Electroencephalography (EEG) in Python
http://mne.tools
BSD 3-Clause "New" or "Revised" License
0 stars 1 forks source link

should now only read in boxy data file #26

Closed kuziekj closed 4 years ago

kuziekj commented 4 years ago

So this is the start of us addressing the comments in the other PR: https://github.com/mne-tools/mne-testing-data/pull/67

To make things more simple, this PR changes read_raw_boxy so that it now only reads in the raw boxy file, and doesn't require .elp, .mtg, or .evt files. It's also designed to read in either a single boxy file (saved by default as a .txt I believe) or it can read in multiple files (similar to what we had before where it will read the data files for various blocks and montages). Added a new argument to help with this (multi_file=False).

I'll run some tests on this to makes sure style and everything is okay. Feel free to take a look and let me know if anything needs to be changed.

Also, do you think we should add a test file for this to compare the imported data to p_pod? Thought adding that test to this PR would make sense, rather than making a separate PR. Then we could add tests for each new function we add (such as reading .elp, .mtg, or .evt files).

kylemath commented 4 years ago

tutorial crashed, so I commented everything after the loading, and it seems to work,

I am a bit confused about your question about the test, I think the answer is yes, we want to start with a simple PR with just this and a test of it against Ppod, and then make new PR's to add in all the other features we (you) have made,

kylemath commented 4 years ago

so maybe we want to brach off importBOXY, save all the stuff you did, and then merge this with importBOXY for this PR

kylemath commented 4 years ago

also I may have screwed up the commit history above trying to rebase with their master, help please

kuziekj commented 4 years ago

Yeah sorry for the confusing question. I meant to ask should this PR only be concerned with loading the raw boxy data, and should a test for that (compare loading to p_pod) also be included. Then should the other functions be their own PR (load .elp, load .evt, etc) and also include tests for those functions. Even though I didn't ask it properly, you answered what I meant to ask.

Yeah I didn't try it with the tutorial beyond the loading. I'm not surprised it crashes since the raw data object is now very bare bones (only containing the data basically). We probably don't want to include the plot_80 tutorial at this point anyway, until we have all the other functions implemented as well.

I'm working on designing a test for the imported data, and working on a tutorial for it as well (similar to plot_30). I'll upload those files later today. I'll also remove the plot_80 tutorial for now.

Okay, I'll try and figure out the rebasing issue. Seems that rebasing can cause lots of problems...

kylemath commented 4 years ago

Sorry I didn't rebase I was just trying to merge master

On Fri., Jul. 24, 2020, 9:41 a.m. kuziekj, notifications@github.com wrote:

Yeah sorry for the confusing question. I meant to ask should this PR only be concerned with loading the raw boxy data, and should a test for that (compare loading to p_pod) also be included. Then should the other functions be their own PR (load .elp, load .evt, etc) and also include tests for those functions. Even though I didn't ask it properly, you answered what I meant to ask.

Yeah I didn't try it with the tutorial beyond the loading. I'm not surprised it crashes since the raw data object is now very bare bones (only containing the data basically). We probably don't want to include the plot_80 tutorial at this point anyway, until we have all the other functions implemented as well.

I'm working on designing a test for the imported data, and working on a tutorial for it as well (similar to plot_30). I'll upload those files later today. I'll also remove the plot_80 tutorial for now.

Okay, I'll try and figure out the rebasing issue. Seems that rebasing can cause lots of problems...

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/kylemath/mne-python/pull/26#issuecomment-663601258, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA36GFJVRKHV6QUXNWRY6H3R5GTR7ANCNFSM4PFAY6QA .

kuziekj commented 4 years ago

Ah okay. Maybe when we decide to merge, we do a 'squash and merge'?

image

kylemath commented 4 years ago

Yeah

On Fri., Jul. 24, 2020, 10:02 a.m. kuziekj, notifications@github.com wrote:

Ah okay. Maybe when we decide to merge, we do a 'squash and merge'?

[image: image] https://user-images.githubusercontent.com/20359571/88411181-b55f5880-cd94-11ea-802a-7a28c207eeab.png

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/kylemath/mne-python/pull/26#issuecomment-663610823, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA36GFKIV64LPEZUTEY3TRDR5GWBLANCNFSM4PFAY6QA .

kuziekj commented 4 years ago

Alright so I updated this. Added a tutorial about loading boxy data (similar to plot_30). I added an explanation about how to load and name the montage and block files, if used, but maybe this is a bit too much. Let me know.

I also added a test file to compare loading boxy data to p_pod.

I removed the plot_80 tutorial for now, and some references to it, since we can't really use it until we have all the other functions implemented. Maybe we also want to change the tutorial since we know more about the ANC dataset.

kuziekj commented 4 years ago

Alright, updated this so it will only load the default file (.txt) saved by boxy and the tutorial only refers to this file type.