Skip to content

[FIX] Bug fixes from adding KCNI name support#267

Merged
josephmje merged 6 commits intoTIGRLab:masterfrom
DESm1th:kcni
Mar 18, 2020
Merged

[FIX] Bug fixes from adding KCNI name support#267
josephmje merged 6 commits intoTIGRLab:masterfrom
DESm1th:kcni

Conversation

@DESm1th
Copy link
Copy Markdown
Contributor

@DESm1th DESm1th commented Mar 12, 2020

This fixes a problem where scan tags with the substring "PHA" were being flagged as invalid, and adds tests to catch this and other similar tag issues, and also reverts some of the changes made to xnat_fetch_sessions because I forgot that their server's experiments are not meaningful and it isn't really necessary to get them to switch to KCNI format.

@auto-assign auto-assign bot requested review from edickie, jerdra and josephmje March 12, 2020 20:01
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 12, 2020

Codecov Report

Merging #267 into master will increase coverage by 0.23%.
The diff coverage is 57.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #267      +/-   ##
==========================================
+ Coverage   30.81%   31.04%   +0.23%     
==========================================
  Files          54       54              
  Lines        8396     8430      +34     
==========================================
+ Hits         2587     2617      +30     
- Misses       5809     5813       +4
Flag Coverage Δ
#unittests 31.04% <57.69%> (+0.23%) ⬆️
Impacted Files Coverage Δ
bin/xnat_fetch_sessions.py 0% <0%> (ø) ⬆️
datman/scanid.py 85.13% <0%> (ø) ⬆️
tests/test_datman_scanid.py 100% <100%> (ø) ⬆️

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 49ac27c...e2873f9. Read the comment docs.

Comment thread datman/scanid.py
'(?!.+PHA)(?P<subject>[^_]+)_' \
'(?P<subject>[^_]+)(?<!PHA)_' \
'(?P<timepoint>[^_]+)_' \
'(?!MR)(?!SE)(?P<session>[^_]+))'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is the MR and SE here a hack for dealing with phantoms?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That and to ensure KCNI IDs (correctly formed or not) won't match. We can't just restrict to numeric sessions only because of prelapse (and possibly others).

@DESm1th DESm1th requested a review from josephmje March 18, 2020 15:30
@josephmje josephmje merged commit 07e4ac1 into TIGRLab:master Mar 18, 2020
@DESm1th DESm1th deleted the kcni branch October 1, 2020 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants