ENH: Add channel block information by larsoner · Pull Request #33 · mne-tools/fiff-constants

larsoner · 2020-11-25T17:09:14Z

Better solution for channel names > 15 chars.

larsoner

@mkajola @jnenonen I tried to mostly follow your proposal, but added some comments inline about why I made specific choices. Feel free to look on GitHub and comment inline where needed!

Companion PR in MNE is mne-tools/mne-python#8574

larsoner · 2020-11-25T17:09:43Z

DictionaryTags.txt

-#...
-#<free>			3999    -
-#
-#==============================================================================================================


Removed all of this because it's clearer just to point people to the companion file I think.

Sorry, maybe I did not quite get what was the change. I suppose these lines were just a placeholder to show a free tag range?

There is the ch_name_list 3507 farther up, but this is redundant with the definition in the _MNE.txt

larsoner · 2020-11-25T17:10:02Z

DictionaryTags.txt

 ch_unit_mul		257	int32		rel	"Unit multiplier exponent.
 ch_dacq_name		258	string		-	"Name of the channel in the data acquisition system. Corresponds to fiffChInfoRec.name."
-#<reserved>		259
+ch_coil_type		259	int32		-	"Coil type in coil_def.dat"


This was one of two entries that is in the fiffChInfoRec but not in the spec

The # tags are reserved. In practice that means that somewhere in this universe there are files that are using the tags, but they are for some reason withdrawn or not documented. So lets avoid them.

larsoner · 2020-11-25T17:11:37Z

DictionaryTags.txt

+:channel info extension
+#==============================================================================================================
+#<free>			4100
+ch_coord_frame		4101	int32      -	"Channel coordinate frame"


This is one thing that is missing from the channel info. Currently the coord_frame is taken based on the channel type (MEG: device, EEG: head), but it's cleaner just to encode it directly. We're out of constants in the 250-ish range, so I reserved 100 slots for volume (above) and started a new potential range for channel information or other info in the next free block.

Very good point. An explicit coordinate frame is definitely better than implicit. I have nothing against that. New things should use it. Unfortunately if somebody needs backwards compatibility, the reader needs to know the implicit ones.
There seem to be some space available around 350+ (I hope we never have tens of different ways to encode data).
This is also the correct time to specify exactly what each thing means, and think about is this compatible with future. This probably goes to the other pull, but there should be convenient way to have more text on the descriptions.

Unfortunately if somebody needs backwards compatibility, the reader needs to know the implicit ones.

I think this part is okay -- existing readers already implement this logic. So it should be backward compatible, and any new readers will need to be compatible with the (many, many) files without a FIFFB_CH_INFO block anyway.

There seem to be some space available around 350+

I'll push a commit to move them down there.

This is also the correct time to specify exactly what each thing means, and think about is this compatible with future.

At least these two additions seem fairly clear/unambiguous in the context of channel information. But I'll make a comment in the other issue about this more general problem.

mkajola · 2020-11-26T12:51:38Z

This is general one on the development.
Is the current branching scheme enough, if we start to develop the spec further here in Github. That would mean that there will be "experimental" specs, and I would expect that head of the master is "valid". Different approaches are possible, also this one.
Another is question about github - is there comment/discussion area with "branch-scope"? Or is that in pull requests only.

larsoner

@mkajola I've pushed a commit to address your comments, feel free to look again when you get a chance.

Is the current branching scheme enough, if we start to develop the spec further here in Github. That would mean that there will be "experimental" specs, and I would expect that head of the master is "valid".

Typically the way I've seen projects work on GitHub is that master is the branch that all downstream users -- for this project that's MNE-* and MEGIN software mostly -- work to be compatible with. People make Pull Requests (PRs) like this one that have a branch in a fork with some changes that are basically a proposal to improve upstream master in some way. Once everyone agrees, it gets merged and people start making use of it in their code (often after a release is made, though power "users" might not wait, and just rely on master having a stable public API).

And every once in a while an official release is made. fiff-constants doesn't change all that much, so it's not too bad to do a release any time a sufficiently large change is made. I think this would justify releasing a 1.5.0, so I put it under that milestone (and there is nothing else currently): https://github.com/mne-tools/fiff-constants/milestone/1

Another is question about github - is there comment/discussion area with "branch-scope"? Or is that in pull requests only.

This PR is a request to merge my branch within my fork larsoner/rename-block into the upstream fiff-constants/master branch. So in this sense, it already has "branch scope". It is possible to have branches other than master within fiff-constants itself, but it's best to avoid this if possible because

It's easier to think of everyone as working within their own forks to add things to a single branch (master) than to have separate branches within fiff-constants that people might actually be using, and
While I could have made a fiff-constants/rename-block branch and done the same PR process to request to merge the changes into fiff-constants/master, it's more annoying when working with other people to do it this way, and also more risky (people doing git fetch upstream will always see a bunch of branches; I could accidentally push to fiff-constants/master instead of fiff-constants/rename-block, etc.).

The PR workflow I've followed here has become a sort of de facto GitHub public development "standard procedure" I think. There are different ways to do it of course -- git and GitHub are quite flexible. But staying with this semi-standard way of doing things makes troubleshooting and contributing elsewhere easier / more consistent.

larsoner · 2020-11-26T14:00:38Z

DictionaryTags.txt

-#...
-#<free>			3999    -
-#
-#==============================================================================================================


There is the ch_name_list 3507 farther up, but this is redundant with the definition in the _MNE.txt

larsoner · 2020-11-26T14:01:59Z

DictionaryTags.txt

+:channel info extension
+#==============================================================================================================
+#<free>			4100
+ch_coord_frame		4101	int32      -	"Channel coordinate frame"


Unfortunately if somebody needs backwards compatibility, the reader needs to know the implicit ones.

I think this part is okay -- existing readers already implement this logic. So it should be backward compatible, and any new readers will need to be compatible with the (many, many) files without a FIFFB_CH_INFO block anyway.

There seem to be some space available around 350+

I'll push a commit to move them down there.

This is also the correct time to specify exactly what each thing means, and think about is this compatible with future.

At least these two additions seem fairly clear/unambiguous in the context of channel information. But I'll make a comment in the other issue about this more general problem.

mkajola · 2020-12-07T16:52:39Z

Hello Eric,
been bit busy..
I am still not fully home with github, so please excuse me about plenty of questions. I think you are right that for this purpose the current scheme is fine. One can use the (git) tags when one wants to have a "stable" release.

And for the (fiff) tags, they look fine too. One small thing. I think that the data type of ch_coord_frame is enum(coord), and ch_coil_type is enum(coil), so that are bound to a specific value set and enums are ints anyway. The description should be 'application agnostic', so referring coildef.dat is not quite optimal.
I suppose we can keep the coil for now, though nowadays sensor might be more descriptive. We can add aliases later if we wish.
So, almost there.
What would be a good place for rules - like that there must be at least ch_scan_no in the block, and that the block overrides the structure. That does not really fit well in the dictionary, so maybe I just must write these in the manual.

larsoner · 2020-12-08T21:16:44Z

I think that the data type of ch_coord_frame is enum(coord), and ch_coil_type is enum(coil)

Done!

What would be a good place for rules - like that there must be at least ch_scan_no in the block, and that the block overrides the structure. That does not really fit well in the dictionary, so maybe I just must write these in the manual.

Yeah these seem like manual-level entries probably. I don't see a good way to put these in the .txt files currently.

If you and @jnenonen are happy with this version, feel free to click the big green button! I usually do a "Squash and merge" because it puts all the changes into a single commit automatically, which keeps repository size small and makes git bisect work nicely.

mkajola · 2020-12-09T10:28:34Z

Ok, done.

ENH: Add channel block information

5b59352

larsoner commented Nov 25, 2020

View reviewed changes

This was referenced Nov 25, 2020

ENH: Add mne_ch_name_mapping #29

Closed

MRG, ENH: Remove 15-char limit for FIF mne-tools/mne-python#8574

Merged

larsoner mentioned this pull request Nov 26, 2020

ENH: Add other options to FIF #30

Open

4 tasks

STY: Better ranges

40cdd67

larsoner commented Nov 26, 2020

View reviewed changes

FIX: Off by one

fb66d5f

FIX: Enum

bf1db40

mkajola merged commit dca2d1e into mne-tools:master Dec 9, 2020

larsoner deleted the rename-block branch December 9, 2020 12:01

larsoner mentioned this pull request Jul 19, 2021

ENH: Channel tags mne-tools/mne-python#9591

Open

larsoner mentioned this pull request Jul 28, 2021

ENH: Channel tags #36

Open

Conversation

larsoner commented Nov 25, 2020

Uh oh!

larsoner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkajola commented Nov 26, 2020

Uh oh!

larsoner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkajola commented Dec 7, 2020

Uh oh!

larsoner commented Dec 8, 2020

Uh oh!

mkajola commented Dec 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants