FIX: Fix missing enums

[larsoner]

Fixes missing enums.

[larsoner]

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.

[mshamalainen]

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 notifications@github.com wrote:

@LorenzE commented on this pull request.

In DictionaryTypes.txt https://github.com/mne-tools/fiff-constants/pull/8#discussion_r220766016:

@@ -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 https://github.com/mne-tools/fiff-constants/pull/8#pullrequestreview-159250442, 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 GitHub","url":"https://github.com/mne-tools/fiff-constants"}},"updates":{"snippets":[{"icon":"PERSON","message":"@LorenzE commented on #8"}],"action":{"name":"View Pull Request","url":"https://github.com/mne-tools/fiff-constants/pull/8#pullrequestreview-159250442"}}} [ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/mne-tools/fiff-constants/pull/8#pullrequestreview-159250442", "url": "https://github.com/mne-tools/fiff-constants/pull/8#pullrequestreview-159250442", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } }, { "@type": "MessageCard", "@context": "http://schema.org/extensions", "hideOriginalBody": "false", "originator": "AF6C5A86-E920-430C-9C59-A73278B5EFEB", "title": "@LorenzE commented on 8", "sections": [ { "text": "", "activityTitle": "Lorenz Esch", "activityImage": "https://assets-cdn.github.com/images/email/message_cards/avatar.png", "activitySubtitle": "@LorenzE", "facts": [ ] } ], "potentialAction": [ { "targets": [ { "os": "default", "uri": "https://github.com/mne-tools/fiff-constants/pull/8#pullrequestreview-159250442" } ], "@type": "OpenUri", "name": "View on GitHub" }, { "name": "Unsubscribe", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 378948028\n}" } ], "themeColor": "26292E" } ]

[larsoner]

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]

@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]

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]

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]

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]

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]

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]

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]

We don’t have a coil_def.dat file, just continue with your latest one.

[NENONEN_Outlook Signature]

From: Eric Larson notifications@github.com Sent: 5. lokakuuta 2018 17:37 To: mne-tools/fiff-constants fiff-constants@noreply.github.com Cc: Jukka Nenonen jukka.nenonen@megin.fi; Mention mention@noreply.github.com Subject: Re: [mne-tools/fiff-constants] FIX: Fix missing enums (#8)

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

@jnenonenhttps://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 GitHubhttps://github.com/mne-tools/fiff-constants/pull/8#issuecomment-427387919, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AOOx-im70i_6z8eicDbSObqBevh7o8G-ks5uh26ZgaJpZM4Wlkm8.

[LorenzE]

@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.