strip non-alphanumeric from session ids too

[keithcallenberg]

Similar to our subject IDs, many of our session IDs have underscores as well. This causes BIDS validation to fail:

[ERR] Ses label contain an Illegal Character hyphen or underscore. Please edit the filename as per BIDS spec. (code: 63 - SESSION_VALUE_CONTAINS_ILLEGAL_CHARACTER)

The same function in heudiconv that strips non-alphanumeric characters for subject IDs (convert_sid_bids) can be used for session IDs here too.

[codecov[bot]]

Codecov Report

Merging #540 (67e5656) into master (80a6538) will decrease coverage by 0.07%. The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master     #540      +/-   ##
==========================================
- Coverage   77.64%   77.56%   -0.08%     
==========================================
  Files          41       41              
  Lines        3167     3174       +7     
==========================================
+ Hits         2459     2462       +3     
- Misses        708      712       +4     

Impacted Files	Coverage Δ
heudiconv/bids.py	83.56% <66.66%> (-1.05%)	:arrow_down:
heudiconv/convert.py	86.91% <66.66%> (-0.22%)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 80a6538...67e5656. Read the comment docs.

[yarikoptic]

Thank you for the PR! Indeed we should do that! But

• reuse of convert_sid_bids for session might be misleading. If we decide to introduce some subject specific changes to that function, we could affect session cleansing as well.
• let's rename convert_sid_bids to sanitize_label or alike (I do not think it is worth keeping bids for every function in bids module, and we do have a mix ATM), provide a shim for convert_sid_bids which would also issue a deprecation warning suggesting to use sanitize_label (eventually we would remove it after a few releases)
• looking at its code I am a little confused why it unconditionally issues a warning, which IMHO should be issued only if sanitized value differs from original
• we would need to add/adjust some unittest so we trigger/cover this new condition

[yarikoptic]

@keithcallenberg interested to bring this PR to/over the finish line? ;)

[keithcallenberg]

Yes, thanks for the reminder! I will take a stab at this today, following your suggestions.

On Thu, Feb 10, 2022 at 10:49 AM Yaroslav Halchenko < @.***> wrote:

@keithcallenberg https://github.com/keithcallenberg interested to bring this PR to/over the finish line? ;)

— Reply to this email directly, view it on GitHub https://github.com/nipy/heudiconv/pull/540#issuecomment-1035080382, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHT62Z4NW7CSXSTJ5NIP2TU2PNCHANCNFSM5L7FK6ZQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were mentioned.Message ID: @.***>

[keithcallenberg]

@yarikoptic I think I covered all but your last bullet point. Are there any existing tests that deal with invalid subject_ids?

[yarikoptic]

@yarikoptic I think I covered all but your last bullet point. Are there any existing tests that deal with invalid subject_ids?

dunno (would need to look - might be not), but you can add basic unittest for the function within test_bids.py to ensure it does what you expect it to do.

[yarikoptic]

ping @keithcallenberg - interested to bring this PR to/over the finish line? thanks in advance for working on this regardless of that decision! ;)

[l-espana]

Hi! Is there any update to this? We have a lot of longitudinal data and it would be very useful. Happy to help if it's needed!

[keithcallenberg]

Lezlie, if you could add the unit testing to test_bids.py that yarikoptic was mentioning, that would be great! I'm sorry, I haven't had the time.

On Thu, Jul 14, 2022 at 9:50 AM Lezlie Espana @.***> wrote:

Hi! Is there any update to this? We have a lot of longitudinal data and it would be very useful. Happy to help if it's needed!

— Reply to this email directly, view it on GitHub https://github.com/nipy/heudiconv/pull/540#issuecomment-1184473743, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHT625L5NDDSBNQSVABQKTVUALIRANCNFSM5L7FK6ZQ . You are receiving this because you were mentioned.Message ID: @.***>

[l-espana]

Okay, I think I have some testing and some updates, but this is my first time contributing to something like this...what's the best way to commit/push/merge and which branch should I be using? I don't want to mess anything up, ha.

[yarikoptic]

thanks for considering to contribute... not sure what is the best tutorial on how to submit a PR, please check this one out google gave me first https://opensource.com/article/19/7/create-pull-request-github ?