FIX: Fix missing enums #8

larsoner · 2018-09-12T14:58:32Z

Fixes missing enums.

larsoner

Adds a few missing entries from the coil enum in the MEGIN range
Adds two missing units from hte unit enum in the MEGIN range
Adds a new next enum in the MEGIN range (not sure if this makes sense)
Adds a new mne_coord enum in the MNE range

Let me know if this makes sense to you! Feel free to comment in-line on the changes -- I've already highlighted the 4 change categories.

larsoner · 2018-09-12T14:58:58Z

DictionaryTypes.txt

@@ -200,6 +200,8 @@ enum(unit) "FIFF value unit code"
    lx		116	"lux"
    T_m		201	"T/m"
    Am		202	"Am"
+    Am_m2		203 "Am/m^2"
+    Am_m3		204 "Am/m^3"


These are in MNE but missing here. Okay to add to the MEGIN range?

Seems fine to me.

larsoner · 2018-09-12T15:00:04Z

DictionaryTypes.txt

+    artemis123_ref_mag 7502  "Artemis123 reference magnetometer"
+    artemis123_ref_grad 7503  "Artemis123 reference gradiometer"
+    kriss_grad         8001  "KRISS gradiometer"
+    sample_tms_planar  9001  "Sample TMS sensor"


Same -- MNE has these 7 FIFFV_COIL_ types that MEGIN does not have. Okay to add these? I think they have already been used in MNE-C software by @mshamalainen

7003 conflicts with LorenzE suggestion. What is the difference between baby_ref_mag (7003) and baby_ref_mag2 (7003)?

I think his names are better/more complete. I'll see if he can push those changes here, and then reviewing again should be possible / hopefully not so bad.

DictionaryTypes.txt

larsoner · 2018-09-12T15:07:14Z

DictionaryTypes_MNE.txt

+    fs_tal_gtz       2004 "FreeSurfer Talairach (MNI z > 0)"
+    fs_tal_ltz       2005 "FreeSurfer Talairach (MNI z < 0)"
+    fs_tal           2006 "FreeSurfer Talairach"
+}


These MNE coordinate frames in principle could live in the MEGIN range for enum(coord), but it might make sense to leave them separate (done in current PR).

Seems OK, consider later whether to add in MEGIN range.

From @mkajola:

Are “enum(mne_coord) "Additional coordinate type"” used to define coordinate systems ...

In theory these could just be in the main MEGIN range, yes. I'm happy either way.

The advantage/logic to having them in the MNE range is that I don't think they are used in MEGIN software anywhere. In MNE usually these coordinate frames are transient and we put everything in standard Neuromag head, device, or MRI (FreeSurfer RAS) coordinates.

The drawback is that now there are two places that coordinate frame definitions live: the "main" ones in MEGIN range and some "auxiliary" ones in MNE range.

@jnenonen @mkajola if you're okay moving these to the MEGIN range, I'm happy to do it -- it probably does make things simpler in the long run. I don't anticipate needing many more coordinate frame definitions -- maybe a couple if other systems have different head coordinate definitions we need to respect?

larsoner · 2018-09-18T11:18:31Z

Thanks for catching the inconsistencies @jnenonen. I have fixed the "Sequence" to have 0 for next file, and put @LorenzE's better BabyMEG definitions in this file. Ready for another round of review from my end.

larsoner · 2018-09-18T11:19:18Z

DictionaryTypes.txt

@@ -306,6 +315,13 @@ enum(role) "Values for 'ref_role'; the role of a reference"
 }


+enum(next) "File pointers"


Also @jnenonen better description welcome here if you have one!

LorenzE · 2018-09-27T00:50:12Z

DictionaryTypes.txt

+    artemis123_grad    7501  "Artemis123 gradiometer"
+    artemis123_ref_mag 7502  "Artemis123 reference magnetometer"
+    artemis123_ref_grad 7503  "Artemis123 reference gradiometer"
+    kriss_grad         8001  "KRISS gradiometer"


@larsoner Can you pls double check if you switched the value of kriss_grad with sample_tms_planar? The mne-python coil_def.dat https://github.com/mne-tools/mne-python/blob/master/mne/data/coil_def.dat suggest otherwise.

Over in our constants file it's 8001 but in the coil_def I see it's 9001, so looks like our constants file has a bug! I've pushed a commit to fix it here and will make an MNE PR shortly.

mshamalainen · 2018-09-27T02:17:11Z

According to MNE-C KRISS gradiometer should be 9001. How were the Artemis coil definitions created?

…

On Sep 26, 2018, at 8:50 PM, Lorenz Esch ***@***.***> wrote: @LorenzE commented on this pull request. In DictionaryTypes.txt <#8 (comment)>: > @@ -295,7 +297,14 @@ enum(coil) "Sensor coil type" kit_grad 6001 "KIT gradiometer" kit_ref_mag 6002 "KIT reference magnetometer" baby_grad 7001 "Baby-SQUID gradiometer" - baby_mag 7002 "Baby-SQUID magnetometer" + baby_mag_inner 7002 "BabyMEG inner layer magnetometer" + baby_mag_outer 7003 "BabyMEG outer layer magnetometer" + baby_mag_ref 7004 "BabyMEG reference layer magnetometer" + artemis123_grad 7501 "Artemis123 gradiometer" + artemis123_ref_mag 7502 "Artemis123 reference magnetometer" + artemis123_ref_grad 7503 "Artemis123 reference gradiometer" + kriss_grad 8001 "KRISS gradiometer" @larsoner <https://github.com/larsoner> Can you pls double check if you switched the value of kriss_grad with sample_tms_planar? The mne-python coil_def.dat https://github.com/mne-tools/mne-python/blob/master/mne/data/coil_def.dat <x-msg://159/url> suggest otherwise. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAlVajse3SnzO5RWdNPIXtpmPzpR3OuEks5ufCDEgaJpZM4Wlkm8>. {"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/mne-tools/fiff-constants","title":"mne-tools/fiff-constants","subtitle":"GitHub repository","main_image_url":"https://assets-cdn.github.com/images/email/message_cards/header.png","avatar_image_url":"https://assets-cdn.github.com/images/email/message_cards/avatar.png","action":{"name":"Open in ***@***.*** commented on #8"}],"action":{"name":"View Pull Request","url":"#8 (review)"}}} [ { ***@***.***": "http://schema.org", ***@***.***": "EmailMessage", "potentialAction": { ***@***.***": "ViewAction", "target": "#8 (review)", "url": "#8 (review)", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { ***@***.***": "Organization", "name": "GitHub", "url": "https://github.com" } }, { ***@***.***": "MessageCard", ***@***.***": "http://schema.org/extensions", "hideOriginalBody": "false", "originator": "AF6C5A86-E920-430C-9C59-A73278B5EFEB", "title": ***@***.*** commented on 8", "sections": [ { "text": "", "activityTitle": "**Lorenz Esch**", "activityImage": "https://assets-cdn.github.com/images/email/message_cards/avatar.png", "activitySubtitle": ***@***.***", "facts": [ ] } ], "potentialAction": [ { "targets": [ { "os": "default", "uri": "#8 (review)" } ], ***@***.***": "OpenUri", "name": "View on GitHub" }, { "name": "Unsubscribe", ***@***.***": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 378948028\n}" } ], "themeColor": "26292E" } ]

larsoner · 2018-09-27T02:29:03Z

From over in the MNE-Python PR:

# Coil definitions for artemis123 coils make use of integration points
# according to the last formula in section 25.4.62 in the
# "Handbook of Mathematical Functions: With Formulas,
# Graphs, and Mathematical Tables" edited by Abramowitz and Stegun.
# http://people.math.sfu.ca/~cbm/aands/abramowitz_and_stegun.pdf
# They were generated using
# gist: https://gist.github.com/bloyl/84a2dc04d77ef0f300aef397f57201f2

So using the code here.

larsoner · 2018-09-27T02:41:39Z

@jnenonen @mkajola I pushed a commit to unify the "mne_coord" enum with the "coord" enum since it does seem like a cleaner approach. Let me know if this looks reasonable to you and, if so, we can hopefully merge these changes.

mkajola · 2018-10-02T09:51:39Z

Hello @larsoner ,
Looked at the current head of larsoner-missing. I think it more or less there.

Could revert the modernization of Am_m2 & m3. Has some character encoding that looks weird in my emacs. Anyway the dictionary is intended to be machine readable, so at this phase it might better to stay in ascii. Could align the the tabs at the same time.
Same for moles.

Could remove the Finnish comment about "stukturaalista". I think it is not needed.

@mshamalainen just question about old stuff: how does the "MRI voxel coordinates" compare to
"data volume".
It would be nice if DictionaryIOD_MNE had explanations what the blocks mean.

larsoner · 2018-10-02T11:46:33Z

Could revert the modernization of Am_m2 & m3

Yep, I can do that.

Could align the the tabs at the same time.

So far there are tabs used for alignment. Tab-based alignment for variable-length entries -- such as some long names and some short names -- is a bit of a pain because it relies on the tab width being rendered in a standardized way, which in general varies by editor. We could decide how many characters we want a tab stop to be equivalent to, but then everyone has to view the files with these settings. So I'd suggest we change to using spaces. I'll do that for the entries I've changed unless someone objects. We should change the other entries at a later time: #11

how does the "MRI voxel coordinates" compare to "data volume".

From what I understand, in MNE the FIFFV_COORD_MRI / "data volume" coordinates are in units of meters relative to the center of the head (today we usually use the Freesurfer surface RAS coordinates for this). The "mri voxel" coordinates are in units of pixel indices of the volume, i.e., for a 1mm^3 acquisition the units are effectively mm, with the center at one of the volume corners (I think).

It would be nice if DictionaryIOD_MNE had explanations what the blocks mean.

I did not change that file in this PR, so I suggest that we do it in a separate PR. It's generally easier to review in smaller separable chunks, easier me to get to it later, and better to get this PR merged sooner. GitHub facilitates making this sort of small incremental progress. I have opened a new issue for this so we don't forget to do it: #10

larsoner · 2018-10-02T20:22:50Z

Pushed a commit to remove the non-ASCII chars, as well as one to change the tabs to spaces for alignment in this file only.

I also added basic continuous integration (Travis-CI, which is free for open-source projects) to ensure that we don't add non-ASCII chars in the future. You can look at the .travis.yml to see what runs automagically for us, and click on the status badge to look at the result, which I ran on an intentionally bad commit. In case you are not familiar with CI services -- you can have them do all sorts of useful things. For example, you could set it up to remotely build the fiff-support library on any PR you open, and on merge make the binaries available somewhere, etc. In MNE we have a (free) CI service continuously build our docs and upload them (see modified date at the bottom), for example.

@jnenonen @mkajola good to go from your end now? Since there are a ton of whitespace changes the diff looks painful, but if you just look at each of the last few commits (can click on them on GitHub) it's not as painful.

mkajola · 2018-10-04T15:10:12Z

Hi @larsoner,
Looks good to me. Only thing I saw was an old typo Neuormag in Types.txt.
What si now the correct way to proceed?

larsoner · 2018-10-04T15:11:48Z

I would suggest tha you click the big green button. Then you or I (or someone else) opens a PR to fix the typo. If you're interested in learning how to use git and GitHub a typo fix is a nice one to start with. If you aren't interested in doing that, it can wait until someone else gets around to it.

larsoner · 2018-10-05T14:37:12Z

Thanks for the reviews and iterations @mkajola @jnenonen ! I hope to open a PR soon to unify our coil_def.dat files next.

@jnenonen can you email me the latest coil_def.dat you have been using so I can merge the two?

jnenonen · 2018-10-05T14:59:52Z

We don’t have a coil_def.dat file, just continue with your latest one. [NENONEN_Outlook Signature] From: Eric Larson <[email protected]> Sent: 5. lokakuuta 2018 17:37 To: mne-tools/fiff-constants <[email protected]> Cc: Jukka Nenonen <[email protected]>; Mention <[email protected]> Subject: Re: [mne-tools/fiff-constants] FIX: Fix missing enums (#8) Thanks for the reviews and iterations @mkajola<https://github.com/mkajola> @jnenonen<https://github.com/jnenonen> ! I hope to open a PR soon to unify our coil_def.dat files next. @jnenonen<https://github.com/jnenonen> can you email me the latest coil_def.dat you have been using so I can merge the two? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#8 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AOOx-im70i_6z8eicDbSObqBevh7o8G-ks5uh26ZgaJpZM4Wlkm8>.

LorenzE · 2018-10-05T15:21:04Z

@mshamalainen is currently working on an updated version of the coil_def.dat (Artemis is missing in the mne-c and mne-cpp version). I will open PRs to mne-python and mne-cpp when ready. Once merged and all three projects (mne-c, mne-python and mne-cpp) are on the same page we could add the coil_def.dat to this repo as well.

larsoner force-pushed the missing branch 2 times, most recently from 261d7c2 to 00f8db4 Compare September 12, 2018 15:01

larsoner commented Sep 12, 2018

View reviewed changes

This was referenced Sep 12, 2018

MRG: Test constant consistency mne-tools/mne-python#5332

Merged

Add missing BabyMEG constants #9

Closed

larsoner and others added 3 commits September 18, 2018 07:12

FIX: Fix missing enums

41bf6cc

Add missing babymeg constants

a522366

FIX: Fix next

01fff46

larsoner force-pushed the missing branch from cae4b78 to 3c04483 Compare September 18, 2018 11:16

FIX: Modernize superscripts

16e3c77

larsoner force-pushed the missing branch from 3c04483 to 16e3c77 Compare September 18, 2018 11:17

larsoner commented Sep 18, 2018

View reviewed changes

LorenzE reviewed Sep 27, 2018

View reviewed changes

FIX: Switch

911ed71

larsoner mentioned this pull request Sep 27, 2018

MRG: Wrong numbers mne-tools/mne-python#5560

Merged

FIX: Unify coordinate definitions

21733ff

larsoner mentioned this pull request Oct 2, 2018

BUG: Alignment #11

Open

FIX: Eradicate non-ascii chars

260aecb

larsoner force-pushed the missing branch from daf591b to 260aecb Compare October 2, 2018 15:29

larsoner added 3 commits October 2, 2018 11:40

FIX: No more tabs

d0de8c9

TST: Try Travis

2edaaa3

FIX: Bye unicode

2701f36

larsoner force-pushed the missing branch from bf59977 to 2701f36 Compare October 2, 2018 18:48

mkajola merged commit 134eec7 into mne-tools:master Oct 5, 2018

larsoner deleted the missing branch October 5, 2018 14:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX: Fix missing enums #8

FIX: Fix missing enums #8

larsoner commented Sep 12, 2018

larsoner left a comment

larsoner Sep 12, 2018

jnenonen Sep 18, 2018

larsoner Sep 12, 2018

jnenonen Sep 18, 2018

larsoner Sep 18, 2018 •

edited

Loading

larsoner Sep 12, 2018

jnenonen Sep 18, 2018

larsoner Sep 18, 2018

larsoner commented Sep 18, 2018

larsoner Sep 18, 2018

LorenzE Sep 27, 2018

larsoner Sep 27, 2018

mshamalainen commented Sep 27, 2018 via email

larsoner commented Sep 27, 2018

larsoner commented Sep 27, 2018

mkajola commented Oct 2, 2018

larsoner commented Oct 2, 2018

larsoner commented Oct 2, 2018 •

edited

Loading

mkajola commented Oct 4, 2018

larsoner commented Oct 4, 2018

larsoner commented Oct 5, 2018

jnenonen commented Oct 5, 2018 via email

LorenzE commented Oct 5, 2018

		@@ -306,6 +315,13 @@ enum(role) "Values for 'ref_role'; the role of a reference"
		}


		enum(next) "File pointers"

FIX: Fix missing enums #8

FIX: Fix missing enums #8

Conversation

larsoner commented Sep 12, 2018

larsoner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larsoner Sep 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larsoner commented Sep 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshamalainen commented Sep 27, 2018 via email

larsoner commented Sep 27, 2018

larsoner commented Sep 27, 2018

mkajola commented Oct 2, 2018

larsoner commented Oct 2, 2018

larsoner commented Oct 2, 2018 • edited Loading

mkajola commented Oct 4, 2018

larsoner commented Oct 4, 2018

larsoner commented Oct 5, 2018

jnenonen commented Oct 5, 2018 via email

LorenzE commented Oct 5, 2018

larsoner Sep 18, 2018 •

edited

Loading

larsoner commented Oct 2, 2018 •

edited

Loading