MRG: Test constant consistency #5332
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5332 +/- ##
==========================================
+ Coverage 87.67% 88.29% +0.62%
==========================================
Files 360 361 +1
Lines 66898 67068 +170
Branches 11320 11364 +44
==========================================
+ Hits 58654 59220 +566
+ Misses 5445 5023 -422
- Partials 2799 2825 +26 |
| val = [kind, dtype, unit] | ||
| if id_ not in tag_dups: | ||
| assert id_ not in tags, (tags.get(id_), val) | ||
| tags[id_] = val |
There was a problem hiding this comment.
you cannot fuse / merge the 3 with code blocks to avoid the duplication?
There was a problem hiding this comment.
The formats and exceptions differ so it would lead to more branching. They have more things different than they have in common so I don't think it would make things clearer.
|
ok !
|
larsoner
changed the title
larsoner
changed the title
larsoner
changed the title
| 'FIFFV_MNE_COORD_MRI_VOXEL', | ||
| 'FIFFV_MNE_COORD_RAS', | ||
| 'FIFFV_MNE_COORD_TUFTS_EEG', | ||
| ) |
There was a problem hiding this comment.
@agramfort this is the to-do list for fiff-constants now that I have added tests for everything, I cannot find these in that repo :(
But I think it should just require a few more enums in the fiff-constants to get most of these.
mne/io/tests/test_constants.py
Outdated
| this_enum = name.split('_')[1].lower() | ||
| assert val in enums[this_enum], (name, val) | ||
| elif name.startswith('FWD_'): | ||
| # XXX remove from FIFF to forward.py namespace at some point? |
There was a problem hiding this comment.
@agramfort one more decision point: We currently pollute the FIFF namespace with FIFF.FWD_ values that are not part of the FIFF spec and are not written to disk. Can we just make a new FWD. bunch in mne/forward/forward.py for these? This will break things if people use FIFF.FWD_ in their own code somewhere but I doubt it's done.
| d = _frombuffer_rows(fid, tag.size, dtype='>c', shape=shape, rlims=rlims) | ||
| return text_type(d.tostring().decode('utf-8', 'ignore')) | ||
| return text_type(d.tostring().decode('latin1', 'ignore')) |
There was a problem hiding this comment.
FYI @agramfort I saw in the official docs that strings are written this way, not using UTF-8, so I have changed it :(
There was a problem hiding this comment.
ok. Do we test IO with utf8 characters?
There was a problem hiding this comment.
Yep though I just pushed a commit to more explicitly test it
There was a problem hiding this comment.
this will not break the IO of old files?
There was a problem hiding this comment.
It might make some of the characters look different. But either way (what we have in master vs what this PR does) some files are broken on read and write. The difference is that what this PR does follows the FIF spec, what we had before did not. So this should probably be in whats_new.rst under the BUG section.
|
This can be merged while we wait for mne-tools/fiff-constants#8 to go in. Once that's in, I can remove the remaining Currently we just verify that all of our constants are in
|
|
+1 for adding an entry in BUG section.
besides LGTM
|
|
thanks a lot @larsoner ! |
* ENH: Test constant consistency * FIX: Update names * FIX: Tests * FIX: Fix constant checks * FIX: Last fixes in this round * ENH: Make new FWD namespace * FIX: Use codeload * FIX: Better redirect following * FIX: String encoding * FIX: Better alias * FIX: Clearer unicode test * FIX: Ancient pytest * FIX: Old Pytest * DOC: whats_new.rst
This test should pass.
@agramfort look at
XXXin the code to see the inconsistencies identified. I will email the MEGIN folks about this, and open a PR tofiff-constantsto fix them.WIP until we can make some or all of the
XXXbelow go away.