Skip to content

Conversation

@sappelhoff
Copy link
Member

@larsoner a proper test to include would be to add to each pytest.parametrize item the expected channel names that the montage should contain. Do you think that's overkill, or should we do it?

@agramfort
Copy link
Member

you don't have a file to test this?

@sappelhoff
Copy link
Member Author

you don't have a file to test this?

I do, but in the tests we do it as follows:

pytest.param(
partial(read_custom_montage, head_size=None),
(
"NumberPositions= 96\n"
"UnitPosition mm\n"
"Positions\n"
"E01 : 5.288 -3.658 119.693\n"
"E02 : 59.518 -4.031 101.404\n"
"E03 : 29.949 -50.988 98.145\n"
"Labels\n"
"E01 E02 E03\n"
),
make_dig_montage(
ch_pos={
"E01": [0.005288, -0.003658, 0.119693],
"E02": [0.059518, -0.004031, 0.101404],
"E03": [0.029949, -0.050988, 0.098145],
},
),
"elc",
None,
id="new ASA electrode (elc)",
),

☝️ I basically added the gist of the contents of the test file I have here

@agramfort
Copy link
Member

I would personally add the test

@larsoner larsoner requested a review from drammock as a code owner September 11, 2024 16:15
@larsoner
Copy link
Member

Added a test using parts of the failing file and touched the problematic example, marking for merge-when-green, thanks in advance @sappelhoff !

@larsoner larsoner enabled auto-merge (squash) September 11, 2024 16:16
@larsoner larsoner merged commit ebab915 into mne-tools:main Sep 11, 2024
@sappelhoff sappelhoff deleted the hotfix branch September 12, 2024 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants