-
Notifications
You must be signed in to change notification settings - Fork 39
Support to string type digitization #36
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
Conversation
| %% reading info with mne | ||
| [fid, tree, dir] = fiff_open(fname); | ||
| [info, meas] = fiff_read_meas_info(fid, tree); | ||
| disp(info.dig) |
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.
I think this test would pass on main and on this branch. It would be better to have the test fail on main but pass on this PR. So instead of just a disp, could you use a suitable assert or similar that would fail on main but pass here?
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.
Yes, you are right!
In both fail and pass cases, it would just display the info.dig. One way to verify if we exclusively know the correct number of total digitization points, but that is file specific. For example, in a test case, I had 332 points in total. The main detected only 73 (no extra) points but this branch found 332.
Is this sufficient to just add assert(length(info.dig)==332, 'failed') as this is file specific?
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.
Yeah that would work
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.
Thanks. Done it!
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.
Ahh I see what you mean by "file specific". We'll want the unit test to pass for CIs and any devs running the test. Can you create as small a file as possible for this test and add it to this repo data/raw_data_tsss_mc.fif (or whatever you want to name it)?
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.
Added a 1-second-long test file test/data/string_digi_test_data_1s.fif and tested with main and the current branch. With main it gave 73 points, while 332 with the current branch.
| @@ -0,0 +1,10 @@ | |||
|
|
|||
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.
This needs to be formatted similarly to other test files, see this boilerplate in all our test_* functions (this file should also conform to this naming convention)
mne-matlab/matlab/test/test_fiff_make_dir_tree.m
Lines 1 to 7 in e5a2677
| function test_suite=test_fiff_make_dir_tree | |
| try test_functions=localfunctions(); catch | |
| end | |
| initTestSuite; | |
| function test_fiff_make_dir_tree_() | |
| % Test fiff_make_dir_tree.m |
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.
I have formatted it as you suggested; hopefully it works now.
|
Thanks @neurosignal ! |

Due to the changes in digitization from point to string for extra points, it was noticed at MEGIN that MNE left out the new digitization points. So it was required to make changes to support the new digitization type.
Changes were made to fiff_read_meas_info.m and fiff_read_tag.m and tested with the appropriate dataset. Backward compatibility was also tested.