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

BF: Fix concat_images dtype issue #987

Closed
wants to merge 1 commit into from

Conversation

spolcyn
Copy link

@spolcyn spolcyn commented Jan 2, 2021

Fixes #986

@codecov
Copy link

codecov bot commented Jan 2, 2021

Codecov Report

Merging #987 (7da0125) into master (a7e94d6) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #987      +/-   ##
==========================================
- Coverage   91.85%   91.84%   -0.01%     
==========================================
  Files         101      101              
  Lines       12550    12550              
  Branches     2209     2209              
==========================================
- Hits        11528    11527       -1     
  Misses        683      683              
- Partials      339      340       +1     
Impacted Files Coverage Δ
nibabel/funcs.py 80.28% <100.00%> (ø)
nibabel/volumeutils.py 83.94% <0.00%> (-0.20%) ⬇️

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

@effigies
Copy link
Member

effigies commented Jan 4, 2021

The problem here is that get_data_dtype() could be int16 while the correct interpreted dtype could be float, if the scale factors are non-trivial.

This is just to say that I have seen this and will try to look through it soon, but I haven't had time to think about it carefully.

@effigies
Copy link
Member

effigies commented Mar 3, 2022

Apologies for the extremely slow response... 2021 kept getting away from me in a lot of ways, but hopefully I can be a little more organized this year.

Anyway, I've looked at this again, and the fix is going to be quite easy due to some other in-progress work (#1082). If you cherry-pick d4a6fed, you can use get_obj_dtype(img.dataobj) to determine the logical dtype of each image. I would probably then use numpy.result_type on the logical dtype of all images. So something like:

out_dtype = np.result_type(*(get_obj_dtype(img.dataobj) for img in images))
out_data = np.empty(out_shape, dtype=out_dtype)

The reason I'm not going purely on the single input is that you could concat a uint8 image and a float32 image and the result would depend on which one happened to be first.

@spolcyn
Copy link
Author

spolcyn commented Mar 12, 2022

Thanks @effigies. Sounds like it'd be best to wait for that commit and #1082 to be merged before updated this PR to incorporate the suggestions. Does that sync with your thoughts?

@spolcyn spolcyn closed this Oct 17, 2022
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.

Concat_images result not consistent between load/save cycles
2 participants