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

[MRG] Improves delimiter in read_bvals_bvecs #1422

Merged
merged 23 commits into from Mar 3, 2018

Conversation

Projects
None yet
3 participants
@thechargedneutron
Contributor

thechargedneutron commented Feb 10, 2018

Fixes #1417

thechargedneutron added some commits Feb 10, 2018

@codecov-io

This comment has been minimized.

codecov-io commented Feb 12, 2018

Codecov Report

Merging #1422 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1422      +/-   ##
==========================================
+ Coverage   87.41%   87.43%   +0.01%     
==========================================
  Files         238      238              
  Lines       30282    30320      +38     
  Branches     3253     3253              
==========================================
+ Hits        26472    26510      +38     
  Misses       3057     3057              
  Partials      753      753
Impacted Files Coverage Δ
dipy/io/tests/test_io_gradients.py 95.89% <100%> (+3.39%) ⬆️
dipy/io/gradients.py 79.48% <100%> (+3.01%) ⬆️

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 5a8ed85...9384c0a. Read the comment docs.

@skoudoro

Thank you @thechargedneutron for fixing this issue. I made a couple of comments below and I hope it will help.

It would be nice if you can add unit tests. For this test, you need to create a fake file in a temporary folder, apply this function, and check if we have the expected result.

@@ -38,7 +39,11 @@ def read_bvals_bvecs(fbvals, fbvecs):
if isinstance(this_fname, string_types):
base, ext = splitext(this_fname)
if ext in ['.bvals', '.bval', '.bvecs', '.bvec', '.txt', '.eddy_rotated_bvecs', '']:
vals.append(np.squeeze(np.loadtxt(this_fname)))
with open(this_fname, 'rb') as f:

This comment has been minimized.

@skoudoro

skoudoro Feb 12, 2018

Member

I think we do not need to read the file in binary mode, read mode is enough so can you replace rb by r.

with open(this_fname, 'rb') as f:
content = f.readline()
sniffer = csv.Sniffer()
detect_delimiter = sniffer.sniff(str(content.encode("utf-8", 'ignore')))

This comment has been minimized.

@skoudoro

skoudoro Feb 12, 2018

Member

str(content) should be enough if you are in read (r) mode above.

thechargedneutron added some commits Feb 13, 2018

@skoudoro

Thank you for adding this test. Do not hesitate to create bvect and bval file with coma and tabs. it is good to test that too. You are on the right path! keep going!

@@ -58,6 +59,14 @@ def test_read_bvals_bvecs():
# You entered the bvecs on both sides:
npt.assert_raises(IOError, read_bvals_bvecs, fbvecs, fbvecs)
# All possible delimiters should work
bv_file4 = tempfile.NamedTemporaryFile(mode='wt', delete=False)

This comment has been minimized.

@skoudoro

skoudoro Feb 15, 2018

Member

You can use with directive. it will help you to correctly close whatever the OS. To get an idea, look the test test_fetch_data() here and here. I hope it will help you

This comment has been minimized.

@thechargedneutron

thechargedneutron Feb 16, 2018

Contributor

Can you provide an example on how to use with directive here?

with tempfile.NamedTemporaryFile as temp:
    temp.write('4 44' 32')
    a, b = read_bvals_bvecs(temp.name, None)

I used the above but it always raises an error 😢

# All possible delimiters should work
bv_file4 = tempfile.NamedTemporaryFile(mode='wt', delete=False)
bv_file4.write("66 55 33")
bv_file4.close() # Does not close

This comment has been minimized.

@skoudoro

skoudoro Feb 15, 2018

Member

you do not need it. use the with directive.

bv_file4 = tempfile.NamedTemporaryFile(mode='wt', delete=False)
bv_file4.write("66 55 33")
bv_file4.close() # Does not close
bvals = read_bvals_bvecs(bv_file4.name, None)

This comment has been minimized.

@skoudoro

skoudoro Feb 15, 2018

Member

Should be bvals, _ = read_bvals_bvecs(bv_file4.name, None).

Please, create a small bvect file too.

bv_file4.close() # Does not close
bvals = read_bvals_bvecs(bv_file4.name, None)
ans = np.array([66, 55, 33])
os.unlink(bv_file4.name)

This comment has been minimized.

@skoudoro

skoudoro Feb 15, 2018

Member

you do not need it if you use the first recommandation

bvals = read_bvals_bvecs(bv_file4.name, None)
ans = np.array([66, 55, 33])
os.unlink(bv_file4.name)
npt.assert_array_equal(ans, bvals)

This comment has been minimized.

@skoudoro

skoudoro Feb 15, 2018

Member

good ;-)

thechargedneutron added some commits Feb 16, 2018

thechargedneutron added some commits Feb 18, 2018

@thechargedneutron thechargedneutron changed the title from [WIP] Improves delimiter in read_bvals_bvecs to [MRG] Improves delimiter in read_bvals_bvecs Feb 18, 2018

@thechargedneutron

This comment has been minimized.

Contributor

thechargedneutron commented Feb 18, 2018

@skoudoro Please review.

@skoudoro

This comment has been minimized.

Member

skoudoro commented Feb 19, 2018

LGTM @thechargedneutron, good job. I have to fix the last error.

@skoudoro

This comment has been minimized.

Member

skoudoro commented Mar 3, 2018

The last is fixed and merged @thechargedneutron! Can you rebase this PR and I can merge it after that. Thanks !

@thechargedneutron

This comment has been minimized.

Contributor

thechargedneutron commented Mar 3, 2018

There you go 😄 @skoudoro

@skoudoro

This comment has been minimized.

Member

skoudoro commented Mar 3, 2018

Thanks! I look more closely at the pytest PR this weekend, sorry for the delay

@thechargedneutron

This comment has been minimized.

Contributor

thechargedneutron commented Mar 3, 2018

Yeah! Thanks for reviewing 😄

@skoudoro skoudoro merged commit 4b597aa into nipy:master Mar 3, 2018

3 checks passed

codecov/patch 100% of diff hit (target 87.41%)
Details
codecov/project 87.43% (+0.01%) compared to 5a8ed85
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

ShreyasFadnavis pushed a commit to ShreyasFadnavis/dipy that referenced this pull request Sep 20, 2018

Merge pull request nipy#1422 from thechargedneutron/delimiter
[MRG] Improves delimiter in read_bvals_bvecs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment