Skip to content

ENH: Add parser for Siemens "ASCCONV" text format #418

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

Closed
wants to merge 821 commits into from

Conversation

moloney
Copy link
Contributor

@moloney moloney commented Mar 3, 2016

This format is included in most Siemens DICOM files as well as
other MR related files. It contains many important bits of meta
data.

asCoilSelectMeas[0].aFFT_SCALE[10].lRxChannel = 11
asCoilSelectMeas[0].aFFT_SCALE[11].flFactor = 4.69845
asCoilSelectMeas[0].aFFT_SCALE[11].bValid = 1
asCoilSelectMeas[0].aFFT_SCALE[11].lRxChannel = 12
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to add a test for one of these long list of lists type values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently we don't parse these as lists, instead the key ends up being 'asCoilSelectMeas[0].aFFT_SCALE[6].lRxChannel'.

It would be nice to convert these to lists, but then we need to deal with some potential weirdness (like it seems some lists are using 0-based indexing and others are using 1-based indexing).

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I remember now. I guess we could check whether the LHS list exists and fill with None up to the required index if not? As in:

 sGroupArray.anMember[1] = 2

would create an object sGroupArray (if it doesn't exist), create a list sGroupArray.anMember with contents [None, None]... Would be a little bit of work though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that seems reasonable and worthwhile.

Copy link
Member

Choose a reason for hiding this comment

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

I had some ideas - want me to have a go for a couple of hours, or do you already know how to solve it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By all means, please take a crack at it! Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

As you might have predicted, it took more than a couple of hours, but: moloney#3

@matthew-brett
Copy link
Member

Brendan - did you get a chance to look at my PR to your branch?

effigies and others added 26 commits February 4, 2020 11:00
TEST: Add BaseTestCase class to skip TestCases starting with ``_``
effigies and others added 26 commits February 19, 2020 23:30
imagqes --> images
TEST: Drop nose for pytest
DOC: Attempt to find versioneer version when building docs
imagqes --> images
This format is included in most Siemens DICOM files as well as
other MR related files. It contains many important bits of meta
data.
@codecov
Copy link

codecov bot commented Mar 24, 2020

Codecov Report

Merging #418 into master will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #418   +/-   ##
=======================================
  Coverage   91.69%   91.69%           
=======================================
  Files          97       97           
  Lines       12390    12390           
  Branches     2187     2187           
=======================================
  Hits        11361    11361           
  Misses        688      688           
  Partials      341      341           

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 ae39189...ae39189. Read the comment docs.

@moloney
Copy link
Contributor Author

moloney commented Mar 24, 2020

I screwed up the merge here by doing the rebase before I merged Matthew's work, closing in favor of #896

@moloney moloney closed this Mar 24, 2020
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.

9 participants