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

Add new_vol_like convenience function #60

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

Conversation

clin045
Copy link
Contributor

@clin045 clin045 commented Jan 17, 2022

Added convenience function for creating new volumes, analogous to nilearn's image.new_img_like. This is perhaps too trivial to add as a function, but new_img_like is something that I reach for regularly when working with nilearn, and I think it would benefit NIfTI.jl as well.

@codecov
Copy link

codecov bot commented Mar 24, 2022

Codecov Report

Merging #60 (3a58f45) into master (06d4ce3) will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #60      +/-   ##
==========================================
+ Coverage   52.27%   52.36%   +0.08%     
==========================================
  Files           6        6              
  Lines         549      550       +1     
==========================================
+ Hits          287      288       +1     
  Misses        262      262              
Impacted Files Coverage Δ
src/NIfTI.jl 73.14% <100.00%> (+0.25%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@clin045 clin045 marked this pull request as draft March 24, 2022 13:24
Christopher Lin added 4 commits March 24, 2022 09:47
This reverts commit cdda2d5, reversing
changes made to 06d4ce3.
Revert "fix merge conflicts"

This reverts commit cdda2d5, reversing
changes made to 06d4ce3.

further fixing
@clin045 clin045 marked this pull request as ready for review March 24, 2022 14:03
@Tokazama
Copy link
Member

After a bit of a hiatus Im back and reviewing stuff here. I have to think about this one a bit more because NIVolume probably shouldn't be a thing. It requires that this package maintains an independent array type from others in the scientific community so someone doing heavy optimizations elsewhere might be missed here. I'm also not super excited to deal with problems from breaking changes though

@clin045
Copy link
Contributor Author

clin045 commented Mar 29, 2022

What exactly do you mean by that? Forgive me if I'm being naive, but since this is simply a wrapper for arrays + some metadata, wouldn't array optimizations still carry over to NIVolumes? I imagine that most heavy processing steps would begin with dumping out the raw data from the NIVolume anyways -- thus the convenience of having a new_vol_like() function.

@Tokazama
Copy link
Member

If you care for the long version take a look at this demonstration of poorly optimized wrapped arrays for a basic copy method. JuliaArrays/ArrayInterface.jl#235 (comment)

You're right that 80% of the time it won't make a noticeable difference, especially compared to other languages, but I'm greedy and a bit lazy. I want all of my methods to be as fast as possible and I want to maintain as few array types as possible. More importantly, we want to use as many well maintained community packages so we can focus on the neuroscience.

There are some other issues with the current approach but I would rather we focus on getting you a workable solution. Are you just trying to create new instances so you can save to a nifti file

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