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

MincHeader.get_xyzt_units() #1098

Closed
wants to merge 6 commits into from
Closed

Conversation

jennydaman
Copy link

The headers of MINC files have information which specifies the volume units (almost always mm). This PR exposes the data via MincHeader.get_xyzt_units(), which is homologous to Nifti1Header.get_xyzt_units(). But I don't work with 4D MINC so the 't' unit is simply "unknown".

I'm thinking that it would be nice to have get_xyzt_units() part of the header interface.

This PR is kind of a draft? It works, though my intention is to open a discussion and perhaps support get_xyzt_units() for other file formats too.

@pep8speaks
Copy link

pep8speaks commented Apr 1, 2022

Hello @jennydaman, 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 2022-04-01 19:46:07 UTC

@codecov
Copy link

codecov bot commented Apr 1, 2022

Codecov Report

Attention: Patch coverage is 79.31034% with 6 lines in your changes missing coverage. Please review.

Project coverage is 95.27%. Comparing base (e9ed337) to head (ca61800).

Files with missing lines Patch % Lines
nibabel/minc1.py 81.25% 1 Missing and 2 partials ⚠️
nibabel/minc2.py 57.14% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1098      +/-   ##
==========================================
- Coverage   95.37%   95.27%   -0.11%     
==========================================
  Files         207      207              
  Lines       29652    29679      +27     
  Branches     3017     3021       +4     
==========================================
- Hits        28282    28277       -5     
- Misses        933      953      +20     
- Partials      437      449      +12     

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

@jennydaman
Copy link
Author

Possibly convergent work: #567

@jennydaman
Copy link
Author

jennydaman commented Oct 18, 2024

@effigies PTAL or close?

@effigies
Copy link
Member

Sorry, what does PTAL mean?

@jennydaman
Copy link
Author

Please Take A Look (which you just did)

@effigies
Copy link
Member

I assume you're seeing (in the CI output):

  _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
  nibabel/tests/test_image_api.py:234: in validate_data_interface
      img = imaker()
  nibabel/tests/test_image_api.py:608: in <lambda>
      yield lambda: self.loader(img_params['fname']), img_params
  nibabel/dataobj_images.py:504: in from_filename
      return klass.from_file_map(file_map, mmap=mmap, keep_file_open=keep_file_open)
  nibabel/minc1.py:353: in from_file_map
      units = minc_file.get_units()
  nibabel/minc1.py:117: in get_units
      return tuple(d.units.decode('utf-8') for d in self._dims)
  _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
  
  .0 = <list_iterator object at 0x7f929c3f3700>
  
  >   return tuple(d.units.decode('utf-8') for d in self._dims)
  E   AttributeError: 'netcdf_variable' object has no attribute 'units'

@jennydaman
Copy link
Author

Whoops. Looks like too much has changed in the past 2 years and this PR isn't going to work anymore.

Could we continue the conversation in #1116? If we decide get_xyzt_units is a desirable feature to have, I can look into how to support it for MINC.

@jennydaman jennydaman closed this Oct 18, 2024
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