-
Notifications
You must be signed in to change notification settings - Fork 1
Compare output to p_pod and opt3d #21
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
base: importBOXY
Are you sure you want to change the base?
Conversation
kylemath
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, lets keep this as a branch, we can merge new changes into it as needed,
We don't want to include this in our PR unless we have to, since it has the large data files in it
|
Alright, so I updated this to be closer to how the other tests are done (like test_nirs.py). I was running the tests by opening a separate anaconda window, switching to the 'mne-python' directory, activating the 'mnedev' environment, and running the following commands: pytest mne\preprocessing\tests\test_boxy.py::test_boxy_raw As the names suggest, they are comparing the raw, epoched, and evoked data between boxy.py and p_pod. They all pass for me, so hopefully for you as well. There are also some preprocessing changes in boxy.py (stuff I missed from p_pod). I'll work on getting some shorter (~2s) files to use for testing, since it does take awhile to load the full data set. |
|
hey @kuziekj , so to be clear, we are aiming now to merge this branch into our fork and our main PR, I remember one of the developers mentioned we needed something like this, comparing our preprocessing and loading to the current standard, so does this take care of that goal? |
|
Yeah it should take care of that, it compares our outputs to the various outputs of p_pod. Do you think there are other tests we should add? The nirx tests seem to also check for channel names and such, is that something we'd want to add as well? I suppose we already indirectly test to make sure the channels are in the correct order, since the comparison with p_pod would fail otherwise. |
|
yeah might as well do all the things that the nirx example does |
|
okay sounds good. I'll go through and add any tests I missed. |
|
Okay, I added a few more tests. I also made the script ready for when we get our data added to the 'MNE-testing-data' repo, so it should look in the correct spot once that's been added. I'll send you the data again through slack so you can test it out. Once you think this is ready, I'll merge. |
|
can you make the fork + pull request on the test data? |
|
Think it might be okay to merge this for now? I realised we might also have to tweak the tutorial as a result, since the added preprocessing steps we originally missed from p_pod change how the data looks (mostly just the normalisation step). Once this is merged I can make a new branch to handle any other tutorial changes. I imagine we'll just have to changes some scales and rejection thresholds. |
|
I think based on the discussion we are having on the other board, we shouldn't merge this, and this first PR should be reduced to focus only on the reader itself, then we can add a tutorial including this and other stuff in future PR's, what do you think? |
|
Yeah that makes sense, should probably take smaller steps at a time rather than big ones. So the current version of importBOXY is missing a couple processing steps from p_pod that are included in this PR (I think just the normalisation and fixing the first 12 points). Do you think we should add those directly to importBOXY or just wait until we merge this branch? I think it only affects the tests we have comparing p_pod to boxy.py. I suppose it would also affect the tutorial too since it would change the data, but if we aren't adding the tutorial yet then that shouldn't matter. |
|
we'll wait and see what the discussion on the other PR decides, |
709d742 to
cfdfa46
Compare
This is mostly for our own interests right now, but I made a file that will compare the raw data boxy.py returns, along with the epochs and evoked activity, to the data p_pod returns. Mostly modified p_pod to save data at specific points while it processed the 'anc' dataset. It should print a bunch of "SUCCESS" or "FAILURE" statements, but it seems like everything is passing so far. The testing file is '/dev/compare_opt3d.py'.
I only normalised and averaged the p_pod data, to keep things simple for now. We might not want to merge this branch directly, since I was messing around with it a bit so it's disorganised somewhat, but it might be a good starting point for any other tests we have to include for the 'importBOXY' PR.
I merged this with the previous branch that was testing the phase wrapping we added. I'll delete the PR for that branch and we can keep this instead.