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

ENH: Add "element" containers and make dicom wrappers compatible #416

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

moloney
Copy link
Contributor

@moloney moloney commented Mar 3, 2016

Add two main types: ElemDict and ElemList. Each can only store
objects with an attribute value. Standard lookups with
__getitem__ and get return just this value attribute. To
access the underlying object, a get_elem method is provided.

Also updated the DICOM wrappers to provide a get_elem method that
returns the full DICOM element rather than just the value. Added a
__iter__ method that returns the available DICOM keys.

@moloney
Copy link
Contributor Author

moloney commented Mar 3, 2016

This is a bite-sized chunk from #290. The goal is to provide a nice standard way to access meta data structures, especially once they become nested. So instead of having to do something like:

meta['key1'].value['key2'].value[0].value

we can do:

meta['key1']['key2'][0]

If you need to access the full "element" you can use the get_elem method. The dicomwrappers classes were lacking this ability and so I added a get_elem method that returns the full pydicom DataElement object. I also added a __iter__ method that produces the available DICOM keywords for the data set. It may also make sense to add some other dict-like methods, I am thinking at least keys.

@moloney
Copy link
Contributor Author

moloney commented Mar 3, 2016

I think we should make meta data access on the DICOM wrappers the same as meta data access on other Nibabel image classes. Are we okay with making the image classes dict-like for meta data access? If not, we should figure out what we want and implement it for the DICOM wrappers as well.

For the other image classes things are a bit more complicated as there is also the existing "header" attribute which contains some meta data. I guess we could make it so that nii.header['dim_info'] is the same as nii['header']['dim_info']?

@@ -0,0 +1,188 @@
# emacs: -*- mode: python; py-indent-offset: 4; indent-tabs-mode: nil -*-
# vi: set ft=python sts=4 ts=4 sw=4 et:
'''Containers for storing "elements" which have both a core data value as well
Copy link
Member

Choose a reason for hiding this comment

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

Single first line (docstring standard - also makes auto-generated docs look nicer).

@nibotmi
Copy link
Contributor

nibotmi commented Apr 9, 2016

☔ The latest upstream changes (presumably #439) made this pull request unmergeable. Please resolve the merge conflicts.

@pep8speaks
Copy link

pep8speaks commented Mar 19, 2020

Hello @moloney, Thank you for updating!

Cheers! There are no style issues detected in this Pull Request. 🍻 To test for issues locally, pip install flake8 and then run flake8 nibabel.

Comment last updated at 2020-03-23 22:17:20 UTC

Add two main types: 'ElemDict' and 'ElemList'. Each can only store
objects with an attribute 'value'. Standard lookups with
'__getitem__' and 'get' return just this 'value' attribute. To
access the underlying object, a 'get_elem' method is provided.

Also updated the DICOM wrappers to provide a 'get_elem' method that
returns the full DICOM element rather than just the value. Added a
__iter__ method that returns the available DICOM keys.
@codecov
Copy link

codecov bot commented Mar 23, 2020

Codecov Report

Merging #416 into master will decrease coverage by 0.08%.
The diff coverage is 83.06%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #416      +/-   ##
==========================================
- Coverage   91.70%   91.62%   -0.09%     
==========================================
  Files          96       97       +1     
  Lines       12311    12435     +124     
  Branches     2173     2208      +35     
==========================================
+ Hits        11290    11393     +103     
- Misses        684      698      +14     
- Partials      337      344       +7     
Impacted Files Coverage Δ
nibabel/elemcont.py 82.90% <82.90%> (ø)
nibabel/nicom/dicomwrappers.py 90.82% <85.71%> (-0.09%) ⬇️

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 65d5fc6...bd21464. Read the comment docs.

@effigies
Copy link
Member

effigies commented Apr 7, 2020

@moloney Thanks for rebasing this against master.

@matthew-brett Do you have the time to resume reviewing this (and/or #417, #419)?

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.

None yet

5 participants