Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIX] Consistently refer to Neuromag/Elekta/MEGIN #1310

Merged
merged 6 commits into from
Jun 6, 2024

Conversation

sappelhoff
Copy link
Member

@sappelhoff sappelhoff commented Oct 3, 2022

Thanks for your report @robertoostenveld -- I have looked through the spec to see where else we are inconsistently referring to that system that has been owned by three manufacturers.

In this draft PR I have naively changed the naming to Neuromag/Elekta/MEGIN, however:

  1. If we want to go through with this, some metadata field values have to be deprecated. For example ElektaNeuromag is currently a restricted keyword for the MEGCoordinateSystem metadata. We can change it to be NeuromagElektaMegin, and deprecate EletaNeuromag, that is, print a warning when somebody uses that "old" term and recommend they use the new one instead.
  2. if this MEG system changes "owners" in the future again, e.g., to company XYZ, are we then calling it Neuromag/Elekta/MEGIN/XYZ?

This PR can remain a draft until we've discussed these issues.

@codecov
Copy link

codecov bot commented Oct 3, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.97%. Comparing base (70d7750) to head (8f21acb).

Current head 8f21acb differs from pull request most recent head 3b12da1

Please upload reports for the commit 3b12da1 to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1310      +/-   ##
==========================================
- Coverage   88.04%   87.97%   -0.07%     
==========================================
  Files          16       16              
  Lines        1380     1356      -24     
==========================================
- Hits         1215     1193      -22     
+ Misses        165      163       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@effigies
Copy link
Collaborator

effigies commented Oct 4, 2022

Does it make sense to just declare that these are synonyms that SHOULD be used consistently throughout a dataset, and then we use one consistently throughout the spec (linking to the bit about synonyms)?

@sappelhoff
Copy link
Member Author

@robertoostenveld what do you think about the two points mentioned in my OP, and about Chris' point?

Perhaps @ftadel and @Moo-Marc can offer additional opinions?

@robertoostenveld
Copy link
Collaborator

I like the idea of considering them as synonyms, that also aligns with (non-technical) use of these in regular conversations.

With FieldTrip I also suffered from this (also for other companies), which I why I at a certain point adopted the strategy to refer to them as "old/new/newer/evennewer", as that at least puts them in a consistent order. But considering them as synonyms is better, and aligns better with the actual market situation. E.g. Elekta (the company) might not like it nowadays that we still refer to their name in relation to MEG systems that are currently not manufactured by them any more (but by MEGIN), whereas labs that purchased their system a few years back will still have the old brand stickers on the scanner and probably prefer to refer to that system as their "Elekta" system, and not the name it would have had - had they bought it recently.

@robertoostenveld
Copy link
Collaborator

another similar update would be desired for the MEG system that was initially designed at the Kanazawa Institute of Technology (KIT), later produced and supported by Yokogawa and that now continues at the Ricoh company (we have a copying machine of them, not a MEG system). That should be considered the KIT/Yokogawa/Ricoh system

@Moo-Marc
Copy link
Contributor

+1 for synonyms. Could this also apply to keywords? Instead of renaming ElektaNeuromag, could we have Neuromag, Elekta and Megin as synonym keywords?

Even CTF has renamed itself at least 4 times, though people kept calling them CTF, hence its return: CTF, VSM, MISL, CTF MISLP, CTF Neuro Innovations

@sappelhoff sappelhoff changed the title Consistently refer to Neuromag/Elekta/MEGIN [FIX] Consistently refer to Neuromag/Elekta/MEGIN Nov 28, 2023
@sappelhoff sappelhoff marked this pull request as ready for review November 28, 2023 10:32
@sappelhoff
Copy link
Member Author

I decided to go the deprecation route here. Let's discuss the "synonym" idea again when one of our supported manufacturers actually changes their name. The changes here are "bug fixes".

Ready for review / merge @effigies @robertoostenveld @Moo-Marc @Remi-Gau

@sappelhoff sappelhoff added this to the 1.10.0 milestone Nov 28, 2023
@Remi-Gau Remi-Gau added the consistency Spec is (potentially) inconsistent label Dec 22, 2023
Comment on lines +311 to +312
[DEPRECATED](SPEC_ROOT/common-principles.md#definitions).
Dataset curators SHOULD use `NeuromagElektaMEGIN` instead.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're going to deprecate, then we should have checks to warn curators to normalize. It seems this shows up in 7 metadata fields:

$ git grep -B12 _MEGCoordSys | grep \ name:
src/schema/objects/metadata.yaml-  name: AnatomicalLandmarkCoordinateSystem
src/schema/objects/metadata.yaml-  name: DigitizedHeadPointsCoordinateSystem
src/schema/objects/metadata.yaml-  name: EEGCoordinateSystem
src/schema/objects/metadata.yaml-  name: FiducialsCoordinateSystem
src/schema/objects/metadata.yaml-  name: HeadCoilCoordinateSystem
src/schema/objects/metadata.yaml-  name: MEGCoordinateSystem
src/schema/objects/metadata.yaml-  name: NIRSCoordinateSystem

Could make a check in src/schema/rules/checks/deprecations.yaml:

AnatomicalLandmarkCoordinateSystemDeprecation:
  issue:
    code: ELEKTA_NEUROMAG_DEPRECATED
    message: |
        "ElektaNeuromag" as a coordinate system is deprecated.
        Use "NeuromagElektaMEGIN" instead.
    level: warning
  selectors:
    - sidecar.AnatomicalLandmarkCoordinateSystem
  checks:
    - sidecar.AnatomicalLandmarkCoordinateSystem != "ElektaNeuromag"

DigitizedHeadPointsCoordinateSystemDeprecation:
  $ref: rules.checks.deprecations.AnatomicalLandmarkCoordinateSystemDeprecation
  selectors:
    - sidecar.DigitizedHeadPointsCoordinateSystem
  checks:
    - sidecar.DigitizedHeadPointsCoordinateSystem != "ElektaNeuromag"

...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestions @effigies I agree and have created a PR for this, using your code. Please feel free to adjust if that's not what you had in mind. #1848

Copy link
Collaborator

@robertoostenveld robertoostenveld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me.

Copy link
Collaborator

@effigies effigies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the checks will be in another PR, then this seems good to go.

@effigies effigies merged commit 2f933e8 into bids-standard:master Jun 6, 2024
24 of 25 checks passed
@sappelhoff sappelhoff deleted the meg/name branch June 6, 2024 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consistency Spec is (potentially) inconsistent MEG Magnetoencephalography
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] inconsistent specification of Elekta/Neuromag and Neuromag/Elekta/Megin
5 participants