Skip to content

Conversation

@kuziekj
Copy link
Collaborator

@kuziekj kuziekj commented Jun 23, 2020

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.

Copy link
Owner

@kylemath kylemath left a 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

@kuziekj
Copy link
Collaborator Author

kuziekj commented Jul 3, 2020

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
pytest mne\preprocessing\tests\test_boxy.py::test_boxy_epochs
pytest mne\preprocessing\tests\test_boxy.py::test_boxy_evoked

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.

@kuziekj kuziekj requested a review from kylemath July 3, 2020 22:46
@kylemath
Copy link
Owner

kylemath commented Jul 7, 2020

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?

@kuziekj
Copy link
Collaborator Author

kuziekj commented Jul 7, 2020

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.

@kylemath
Copy link
Owner

kylemath commented Jul 8, 2020

yeah might as well do all the things that the nirx example does

@kuziekj
Copy link
Collaborator Author

kuziekj commented Jul 8, 2020

okay sounds good.

I'll go through and add any tests I missed.

@kuziekj
Copy link
Collaborator Author

kuziekj commented Jul 8, 2020

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.

@kuziekj kuziekj requested a review from kylemath July 8, 2020 17:24
@kylemath
Copy link
Owner

kylemath commented Jul 8, 2020

can you make the fork + pull request on the test data?

@kuziekj
Copy link
Collaborator Author

kuziekj commented Jul 17, 2020

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.

@kylemath
Copy link
Owner

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?

@kuziekj
Copy link
Collaborator Author

kuziekj commented Jul 19, 2020

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.

@kylemath
Copy link
Owner

we'll wait and see what the discussion on the other PR decides,

@larsoner larsoner force-pushed the importBOXY branch 3 times, most recently from 709d742 to cfdfa46 Compare November 19, 2020 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants