Switch from using nibabel InTemporaryDirectory to standard library tmpfile module

[jacobdr]

Closes #2587

[arokem]

Thanks for doing this. And thanks for pinging the nibabel GitHub about this as well. I am curious to see if there's some reason I don't know that they implemented it this way.

[codecov]

Codecov Report

Merging #2589 (6f2a667) into master (f1e89b7) will decrease coverage by 0.35%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2589      +/-   ##
==========================================
- Coverage   85.04%   84.68%   -0.36%     
==========================================
  Files         129      130       +1     
  Lines       17613    17752     +139     
  Branches     2999     3015      +16     
==========================================
+ Hits        14979    15034      +55     
- Misses       1934     2013      +79     
- Partials      700      705       +5     

Impacted Files	Coverage Δ
dipy/direction/peaks.py	84.50% <100.00%> (ø)
dipy/io/gradients.py	95.55% <100.00%> (-0.19%)	⬇️
dipy/reconst/qti.py	92.83% <0.00%> (-6.73%)	⬇️
dipy/data/fetcher.py	40.56% <0.00%> (-2.65%)	⬇️
dipy/data/__init__.py	80.41% <0.00%> (ø)
dipy/viz/plotting.py	13.33% <0.00%> (ø)

[matthew-brett]

Thanks for doing this. And thanks for pinging the nibabel GitHub about this as well. I am curious to see if there's some reason I don't know that they implemented it this way.

Well - as @effigies says in nipy/nibabel#1102 (comment) - once you've accepted the premise in the class name - that the code context involves changing to temporary directory - I don't think there's any other way to implement it. But maybe there is - happy to hear if so.

Or you might ask - why write tests that involve changing the directory? I guess there are various reasons - just to save some code for prefixing the path to lots of temporary files, and testing a process that creates something in the working directory, were two things I thought of.

[pep8speaks]

Hello @jacobdr, Thank you for updating !

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated at 2022-05-25 17:14:36 UTC

[skoudoro]

Hi @jacobdr,

Many tests are failing, can you look at what is going wrong like here?

Thanks!

[jacobdr]

@skoudoro sorry to take so long to get back to this. Just expanded the test coverage in my most recent commits and we'll see how the tests fare now. I was trying to find a way to not have to write back out to disk for performance reasons but I think that is outside the scope of this ticket... so I kept it pretty low-risk and faithful to the OG implementation

[jacobdr]

@matthew-brett @skoudoro can I request a re-review given the changes I made to expand the test coverage please? (also need an approval on the pending GH actions workflow for CI)

[skoudoro]

Hi @jacobdr,

Your tests are in the wrong file. See below for additional details.
Thank you in advance for the update.

ps: failing CIs are not related to your PR now, it should be fixed in #2595

[skoudoro]

Hi @jacobdr,

This PR is almost done, it would be great if you could rebase this PR and address the last comment.
Please, keep us updated. Thank you!

[jacobdr]

@skoudoro recent commits:

• merged master
• reverted all changes to test_gradients in deference to dipy/io/tests/test_io_gradients.py as you astutely pointed out I had re-invented the wheel. the lack of change in coverage should have tipped me off.

[skoudoro]

Thanks @jacobdr for doing this!

Thanks @matthew-brett @effigies @arokem for the review.

merging