Skip to content

Conversation

effigies
Copy link
Member

@effigies effigies commented Jan 6, 2023

Since this much is done, I'm putting it up here. If anybody is up for a review before the release, this can go in 5.0. If not, it can wait for 5.1.

SpatialImage is next on my list, but I didn't want to hold this up if it could get in.

@codecov
Copy link

codecov bot commented Jan 6, 2023

Codecov Report

Base: 92.37% // Head: 92.38% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (570101b) compared to base (79f968f).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1178      +/-   ##
==========================================
+ Coverage   92.37%   92.38%   +0.01%     
==========================================
  Files          97       97              
  Lines       12267    12284      +17     
  Branches     2526     2526              
==========================================
+ Hits        11332    11349      +17     
  Misses        614      614              
  Partials      321      321              
Impacted Files Coverage Δ
nibabel/arrayproxy.py 100.00% <100.00%> (ø)
nibabel/dataobj_images.py 95.16% <100.00%> (+0.42%) ⬆️
nibabel/filebasedimages.py 92.85% <100.00%> (+0.23%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@effigies effigies force-pushed the type/primary_image_api branch from 2859287 to cb85b94 Compare January 6, 2023 17:34
@effigies effigies force-pushed the type/primary_image_api branch from cb85b94 to 05a0b2f Compare January 6, 2023 17:41
Copy link
Contributor

@ZviBaratz ZviBaratz left a comment

Choose a reason for hiding this comment

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

Assuming I'm just missing something in the first comment re /, I think it looks great.

Also, I think using:

CACHING_OPTIONS = ty.Literal['fill', 'unchanged']
MMAP_OPTIONS = ty.Literal['c', 'r']

and maybe even:

HeaderType = FileBasedHeader | ty.Mapping | None

could be nice, in terms of more concise function signatures and code readability. But it might also be too much. Your call.

@effigies effigies force-pushed the type/primary_image_api branch from 9ddf0db to 570101b Compare January 6, 2023 19:31
@effigies effigies merged commit b5a4b8c into nipy:master Jan 6, 2023
@effigies effigies deleted the type/primary_image_api branch January 6, 2023 20:14
@effigies effigies added this to the 5.0.0 milestone Jan 7, 2023
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.

2 participants